Bug 136860 – Local OSR availability calculation should be reusable

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 238190 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:139:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:146:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:152:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:158:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 4 files


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

[Filip Pizlo]

(In reply to comment #2)
> Attachment 238190 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:139:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:146:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:152:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:158:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 4 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I fixed all of these locally.

[Oliver Hunt]

Comment on attachment 238190 [details]
the patch

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

r=me with the reference change

> Source/JavaScriptCore/ChangeLog:19
> +        This reduces the amount of style points one could conceivably get in the future when
> +        hacking JSC, but creating a single reusable thingy for computing local OSR availability.

The last half of this sentence doesn't seem to make sense - early finish?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:6747
> +    Operands<Availability> availability() { return m_availabilityCalculator.m_availability; }
> +    

const Operands<Availability>&

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 238190 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238190&action=review
> 
> r=me with the reference change
> 
> > Source/JavaScriptCore/ChangeLog:19
> > +        This reduces the amount of style points one could conceivably get in the future when
> > +        hacking JSC, but creating a single reusable thingy for computing local OSR availability.
> 
> The last half of this sentence doesn't seem to make sense - early finish?

s/but/by

I will fix.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:6747
> > +    Operands<Availability> availability() { return m_availabilityCalculator.m_availability; }
> > +    
> 
> const Operands<Availability>&

Why?  What does it buy us?

I don't like adding const unless I have a story for what it will accomplish, since pointless uses of const run the risk of just requiring more code to be written (const/non-const variants of things...).  In this case, you can already edit the availability by doing m_availabilityCalculator.m_availability.

A humorous example of when you last asked me to make something const was Node::child1()/child2()/child3().  The result was that everyone would wanted to edit children had to be more verbose - Node->children.setChild1(...).  This wasn't a good thing.

I take it as a given that the purpose of a compiler's internal data structures is that they may be edited.  There is usually no point in dragging const around.

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 238190 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=238190&action=review
> > 
> > r=me with the reference change
> > 
> > > Source/JavaScriptCore/ChangeLog:19
> > > +        This reduces the amount of style points one could conceivably get in the future when
> > > +        hacking JSC, but creating a single reusable thingy for computing local OSR availability.
> > 
> > The last half of this sentence doesn't seem to make sense - early finish?
> 
> s/but/by
> 
> I will fix.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:6747
> > > +    Operands<Availability> availability() { return m_availabilityCalculator.m_availability; }
> > > +    
> > 
> > const Operands<Availability>&
> 
> Why?  What does it buy us?
> 
> I don't like adding const unless I have a story for what it will accomplish, since pointless uses of const run the risk of just requiring more code to be written (const/non-const variants of things...).  In this case, you can already edit the availability by doing m_availabilityCalculator.m_availability.
> 
> A humorous example of when you last asked me to make something const was Node::child1()/child2()/child3().  The result was that everyone would wanted to edit children had to be more verbose - Node->children.setChild1(...).  This wasn't a good thing.
> 
> I take it as a given that the purpose of a compiler's internal data structures is that they may be edited.  There is usually no point in dragging const around.

*facepalm*

Discussion on irc revealed that I was using Operands<Availability> without a &.  Ollie recommends Operands<Availability>& (i.e. non-const ref).  I will do that.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/173672