... because CallFrame.h is the more appropriate place for the CallFrameHeader info.
Created attachment 283119 [details] proposed patch.
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.
Comment on attachment 283119 [details] proposed patch. Let me fix the build error on the debug build first.
Created attachment 283180 [details] proposed patch.
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.
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.
(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?
> 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.
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.
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.
Created attachment 283208 [details] Patch for landing.
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.
Landed in r203006: <http://trac.webkit.org/r203006>.