[webkit-reviews] review granted: [Bug 69690] DFG does not have flow-sensitive intraprocedural control flow analysis : [Attachment 110430] the patch - merged up to ToT

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 69690: DFG does not have flow-sensitive intraprocedural control flow
analysis
https://bugs.webkit.org/show_bug.cgi?id=69690

Attachment 110430: the patch - merged up to ToT
https://bugs.webkit.org/attachment.cgi?id=110430&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=110430&action=review


r+.  I still don't think AbstractValue is really descriptive enough, but I
don't have a better name to suggest.

> Source/JavaScriptCore/dfg/DFGAbstractValue.h:312
> +struct AbstractValue {

AbstractValue doesn't seem to be the right name for this class.  This struct
can tell you a type, not a value - e.g. "int", or "object with properties x,
y", not '3' or '"Hello, world"'.
This naming is confusing in it's uses, e.g. in GetLocal.

> Source/JavaScriptCore/dfg/DFGNode.h:715
> +    unsigned takenBytecodeOffsetDuringParsing()

This change could probably have been made as a separate patch, breaking up this
review a bit would have helped! - probably no point doing so now though.
It's a bit unfortunate that the meaning of the branch's opInfos changes over
time, it would be better if we could guard against error here.
We could consider extra state in debug builds that we can ASSERT on, or we
could change reflect this in the node types (have a
BranchDuringParse/JumpDuringParse that are updated as the successors are set,
and never reach generation).