[Webkit-unassigned] [Bug 24986] [multi-patch] ARM JIT port

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 11 23:31:04 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=24986


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34560|review?                     |review+
               Flag|                            |




--- Comment #100 from Gavin Barraclough <barraclough at apple.com>  2009-08-11 23:30:58 PDT ---
(From update of attachment 34560)
> #define WTF_USE_CONSTANT_POOL 1

Gah, I'm really sorry, but I'm going to have to pick you up on an annoying
technicality.

We should not be defining WTF_USE_ macros outside of Platform.h (there is
another example of this being done, but it's a bug.  you'll see that ENABLE_,
WTF_PLATFORM_, etc are only defined in Platform.h).  The problem generally with
defining these values in other headers is that the macros allow the value no to
be defined, which could lead to unexpected behaviour.  Consider, for example,
if '#define ENABLE_JIT 1' was defined in JIT.h, rather than Platform.h.  For
any .cpp files that include JIT.h the expression 'ENABLE(JIT)' will be true,
but for any files that happen not to include JIT.h this will evaluate to false.

Whilst there is no bug in the way you've used this, or any clear way that it
could lead to a bug in this case, I think we want to keep the design rule that
the USE() etc marco are only used with values defined in Platform.h.

The way that you've structured the macros (defining the value in
AssemblerBufferWithConstantPool.h) seems perfectly sensible, so rather than
trying to move this up into Platform.h I'd suggest you may just want to
consider a quick rename & stop using the USE macro in this case - just switch
'#define WTF_USE_CONSTANT_POOL 1' to something like '#define
ASSEMBLER_HAS_CONSTANT_POOL 1' and replace 'USE(CONSTANT_POOL)' with
'defined(ASSEMBLER_HAS_CONSTANT_POOL)'.

I'm going to call this an r+-with-changes, since everything else looks good. 
If you update a new patch with the macro name fixed one way or another I'll get
it landed for you.

BTW, I'm guessing this may be your intention already, but let's make this the
last change on this ├╝ber-bug - from now on I think all future patch should get
a bug of their own, as per normal process.

cheers,
G.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list