Bug 156454 – Clean up how we reason about the states of AccessCases

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 276109 [details]
work in progress

[Filip Pizlo]

Created attachment 276115 [details]
the patch

[WebKit Commit Bot]

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.

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in r199297