Bug 159549 – Move CallFrame header info from JSStack.h to CallFrame.h

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

[Mark Lam]

... because CallFrame.h is the more appropriate place for the CallFrameHeader info.

[Mark Lam]

Created attachment 283119 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 283119 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 283119 [details]
proposed patch.

Let me fix the build error on the debug build first.

[Mark Lam]

Created attachment 283180 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 283180 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 283180 [details]
proposed patch.

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

> Source/JavaScriptCore/interpreter/CallFrame.h:74
> +    struct CallFrameHeader {
> +        enum Entry {

This is odd C++ style.

The idiomatic way to namespace an enum is "enum class".

The idiomatic equivalent for what you've done is "enum class CallFrameHeaderEntry". I think that's probably better.

"CallFrameHeaderEntry" a bit long, so maybe just "CallFrameSlot" instead. I'm not sure any clients care that these values form a header, and in fact they're not quite a header since more data (function arguments) lies beyond them.

[Mark Lam]

(In reply to comment #6)
> Comment on attachment 283180 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283180&action=review
> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:74
> > +    struct CallFrameHeader {
> > +        enum Entry {
> 
> This is odd C++ style.
> 
> The idiomatic way to namespace an enum is "enum class".
> 
> The idiomatic equivalent for what you've done is "enum class
> CallFrameHeaderEntry". I think that's probably better.
> 
> "CallFrameHeaderEntry" a bit long, so maybe just "CallFrameSlot" instead.
> I'm not sure any clients care that these values form a header, and in fact
> they're not quite a header since more data (function arguments) lies beyond
> them.

I wanted to use an enum class at first, but that resulted in the enum values not being accepted as int values every where they are used.  For example, indexing with [CallFrameHeader::Callee] will yield an error saying that the indexing value, CallFrameHeader::Callee, is not an int.

I have already tried all of these:
    enum class CallFrameEntry { ...
    enum class CallFrameEntry : int { ...
    enum class CallFrameEntry : int {
        CallerFrameAndPCSize = ...
        CodeBlock = CallerFrameAndPCSize,
        Callee = CodeBlock + 1,
        ...

All of them did not resolve the error.  Hence, I fell back on how the pattern which we used previously i.e. an enum inside a class so that the class provides scoping for the enum while the enum values still int type.

As for the name of the class/struct, I'm not that comfortable with CallFrameHeader either given that it is more so a container for CallFrameConstants.  I kept CallFrameHeader to retain the legacy reference to what this values are for, but am open to changing it.

In light of enum class being not a viable solution, what do you propose I do?

[Geoffrey Garen]

> I wanted to use an enum class at first, but that resulted in the enum values
> not being accepted as int values every where they are used.

That's right. enum class is type-safe, so you need to cast or invoke the int constructor to turn it into an int. Will casting work? I think it will.

It looks like our APIs that currently accept JSStack::CallFrameHeaderEntry are reasonably encapsulated, so they can do the casting for us.

[Mark Lam]

Created attachment 283200 [details]
revised patch with Geoff's feedback.

After talking with Geoff offline, we decided to go with static const int fields. This has been applied to the revised patch.

[WebKit Commit Bot]

Attachment 283200 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 283208 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 283208 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

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