Bug 209637 – [JSC] Make Operator an enum class to avoid Op* identifiers

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

[Ross Kirsling]

[JSC] Make Operator an enum class to avoid Op* identifiers

[Ross Kirsling]

Created attachment 394685 [details]
Patch

[Ross Kirsling]

Comment on attachment 394685 [details]
Patch

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

> Source/JavaScriptCore/parser/Nodes.h:1416
> -        unsigned m_operator : 30;
> +        Operator m_operator;

A note of interest:
Looks like this was overlooked in r233937 due to the obfuscation introduced in r170381.
(Not sure if these booleans still need the `: 1` either but I'll leave 'em unless someone says otherwise.)

[Darin Adler]

Comment on attachment 394685 [details]
Patch

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

I like this, but since it’s a coding style thing, and sometimes the people working on JavaScript don’t agree with me on coding style (and I try not to be heavy handed about my opinions in such cases) I am not sure if my review is sufficient.

>> Source/JavaScriptCore/parser/Nodes.h:1416
>> +        Operator m_operator;
> 
> A note of interest:
> Looks like this was overlooked in r233937 due to the obfuscation introduced in r170381.
> (Not sure if these booleans still need the `: 1` either but I'll leave 'em unless someone says otherwise.)

So this should reduce the bits from 30 to 8. But we could limit the number of bits even further if we needed to? I’d want Yusuke to weigh on whether a slight reduction in this node’s size would have a desirable effect and if so, then do what it takes to use the smaller number of bits.

[Yusuke Suzuki]

Comment on attachment 394685 [details]
Patch

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

> Source/JavaScriptCore/parser/Nodes.h:79
> +    enum class Operator : uint8_t {
> +        Equal,
> +        PlusEq,
> +        MinusEq,
> +        MultEq,
> +        DivEq,
> +        PlusPlus,
> +        MinusMinus,
> +        BitAndEq,
> +        BitXOrEq,
> +        BitOrEq,
> +        ModEq,
> +        PowEq,
> +        LShift,
> +        RShift,
> +        URShift
>      };
>      
> -    enum LogicalOperator : uint8_t {
> -        OpLogicalAnd,
> -        OpLogicalOr
> +    enum class LogicalOperator : uint8_t {
> +        And,
> +        Or
>      };

I think this is nice. But please keep in mind about one possible issue (currently we are not hitting this).
IIRC, windows.h is notorious about defining many generic-named macros. So, if you define `Operator::VOID`, you will see some super fancy compile error because VOID is replaced with something fun.

>>> Source/JavaScriptCore/parser/Nodes.h:1416
>>> +        Operator m_operator;
>> 
>> A note of interest:
>> Looks like this was overlooked in r233937 due to the obfuscation introduced in r170381.
>> (Not sure if these booleans still need the `: 1` either but I'll leave 'em unless someone says otherwise.)
> 
> So this should reduce the bits from 30 to 8. But we could limit the number of bits even further if we needed to? I’d want Yusuke to weigh on whether a slight reduction in this node’s size would have a desirable effect and if so, then do what it takes to use the smaller number of bits.

I think the effect is not significantly large. These memory allocation is transient one, so, they are destroyed once the code-generation is done.
But on the other hand, this is kept in memory-pool, and for very small daemons using JSC, once of the memory peak is when parsing code. But maybe not so large.
Reducing memory typically becomes significant effective if the data structure becomes somewhat persistent, like, some sort of cache, data structure which is kept alive so long etc.
But making things small is always better :)

[EWS]

Committed r259150: <https://trac.webkit.org/changeset/259150>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394685 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60995714>