Bug 201966 – Replace JSValue #defines with static constexpr values.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

And also rename them to something more meaningful or more concise where possible.

[Mark Lam]

Created attachment 379098 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 379098 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=379098&action=review

r=me

> Source/JavaScriptCore/runtime/JSCJSValue.h:442
> +    static constexpr int32_t OtherTag       = 0x2ll;
> +    static constexpr int32_t BoolTag        = 0x4ll;
> +    static constexpr int32_t UndefinedTag   = 0x8ll;
>      // Combined integer value for non-numeric immediates.
> -    #define ValueFalse     (TagBitTypeOther | TagBitBool | false)
> -    #define ValueTrue      (TagBitTypeOther | TagBitBool | true)
> -    #define ValueUndefined (TagBitTypeOther | TagBitUndefined)
> -    #define ValueNull      (TagBitTypeOther)
> -
> -    // TagMask is used to check for all types of immediate values (either number or 'other').
> -    #define TagMask (TagTypeNumber | TagBitTypeOther)
> -
> +    static constexpr int32_t ValueFalse     = OtherTag | BoolTag | false;
> +    static constexpr int32_t ValueTrue      = OtherTag | BoolTag | true;
> +    static constexpr int32_t ValueUndefined = OtherTag | UndefinedTag;
> +    static constexpr int32_t ValueNull      = OtherTag;
> +
> +    static constexpr uint64_t MiscTag = OtherTag | BoolTag | UndefinedTag;

I would like to have some comments why this is defined as int32_t while the other ones are defined as uint64_t.

> Source/JavaScriptCore/runtime/JSCJSValue.h:453
> +    static constexpr uint64_t ValueEmpty   = 0x0ll;
> +    static constexpr uint64_t ValueDeleted = 0x4ll;

Can you remove ll here?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:428
> +#endif

Can I have rationale comment here?

[Mark Lam]

Comment on attachment 379098 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=379098&action=review

Thanks for the review.

>> Source/JavaScriptCore/runtime/JSCJSValue.h:442
>> +    static constexpr uint64_t MiscTag = OtherTag | BoolTag | UndefinedTag;
> 
> I would like to have some comments why this is defined as int32_t while the other ones are defined as uint64_t.

The use of int32_t is because those values are being used as TrustedImm32 which expect 32-bit values.  Since these values don't have any bits set above 32-bits, defining them as int32_t saves us from doing static_cast<int32_t> in many places.  I'll add a ChangeLog comment to comment this.

I'll also change all the uint64_ts to int64_ts since that's the type of their original #define values.

>> Source/JavaScriptCore/runtime/JSCJSValue.h:453
>> +    static constexpr uint64_t ValueDeleted = 0x4ll;
> 
> Can you remove ll here?

Will do.

>> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:428
>> +#endif
> 
> Can I have rationale comment here?

The reason for this is because the ARM64 assembler is able to emit a more efficient instruction for loading the constant.  On x86_64, the 2 instructions to construct DoubleEncodeOffset using a shift is more efficient.  I'll also add a ChangeLog comment for this.

[Mark Lam]

Created attachment 379105 [details]
patch for landing.

[Mark Lam]

Created attachment 379107 [details]
patch for landing.

[Mark Lam]

Landed in r250094: <http://trac.webkit.org/r250094>.

[Radar WebKit Bug Importer]

<rdar://problem/55521813>