[webkit-reviews] review granted: [Bug 201966] Replace JSValue #defines with static constexpr values. : [Attachment 379098] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 201966: Replace JSValue #defines with static constexpr values.
https://bugs.webkit.org/show_bug.cgi?id=201966

Attachment 379098: proposed patch.

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 379098
  --> https://bugs.webkit.org/attachment.cgi?id=379098
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?