Patch forthcoming.
Created attachment 276109 [details] work in progress
Created attachment 276115 [details] the patch
Attachment 276115 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:60: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:68: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:77: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:226: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:227: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:228: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 6 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 276115 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=276115&action=review r=me with fixes. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:778 > +void AccessCase::generateImpl(AccessGenerationState& state) > +{ Maybe add an "ASSERT(m_state == Generated);" here because this functions relies on its callers setting m_state accordingly. Should a new caller be added in the future, we would want to ensure that this does not get overlooked. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:821 > - > + Please undo. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1613 > - > + Please undo this. > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:230 > + // Is it still possible for this case to ever be taken? Must call this as a prerequisite for > + // calling generate() and friends. If this returns true, then you can call generate(). If > + // this returns false, then generate() will crash. You must call generate() in the same epoch > + // as when you called couldStillGenerate(). > bool couldStillSucceed() const; Either the comment has a typo (/couldStillGenerate/couldStillSucceed/) or you wanted to rename this function but forgot to. In light of this function being the guard to generate, I think canStillGenerate() would be a better name (I suggest "can" instead of "could" because it's a statement about the present state, not the past). Please fix. > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:255 > + // Perform any action that that must be performed before the end of the epoch in which the case double "that". Please remove one. > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:407 > - > + Please undo this.
(In reply to comment #4) > Comment on attachment 276115 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276115&action=review > > r=me with fixes. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:778 > > +void AccessCase::generateImpl(AccessGenerationState& state) > > +{ > > Maybe add an "ASSERT(m_state == Generated);" here because this functions > relies on its callers setting m_state accordingly. Should a new caller be > added in the future, we would want to ensure that this does not get > overlooked. Good idea. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:821 > > - > > + > > Please undo. OK. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1613 > > - > > + > > Please undo this. OK. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:230 > > + // Is it still possible for this case to ever be taken? Must call this as a prerequisite for > > + // calling generate() and friends. If this returns true, then you can call generate(). If > > + // this returns false, then generate() will crash. You must call generate() in the same epoch > > + // as when you called couldStillGenerate(). > > bool couldStillSucceed() const; > > Either the comment has a typo (/couldStillGenerate/couldStillSucceed/) or > you wanted to rename this function but forgot to. In light of this function > being the guard to generate, I think canStillGenerate() would be a better > name (I suggest "can" instead of "could" because it's a statement about the > present state, not the past). Please fix. I'll fix the comment since I want to err on not changing trunk. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:255 > > + // Perform any action that that must be performed before the end of the epoch in which the case > > double "that". Please remove one. OK. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:407 > > - > > + > > Please undo this. OK.
Landed in r199297