And also rename them to something more meaningful or more concise where possible.
Created attachment 379098 [details] proposed patch.
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?
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.
Created attachment 379105 [details] patch for landing.
Created attachment 379107 [details] patch for landing.
Landed in r250094: <http://trac.webkit.org/r250094>.
<rdar://problem/55521813>