Bug 85945

Created attachment 140849 [details] the patch

Comment on attachment 140849 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=140849&action=review In general this patch looks fine (aside from the bool vs. enum issue i mention), but i'm somewhat tired so don't feel sufficiently focussed to r+ it. Will re-review in the morning. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:114 > - VariableAccessData* newVariableAccessData(int operand) > + VariableAccessData* newVariableAccessData(int operand, bool isCaptured) We try to use enums rather than bools in cases like this as they're more descriptive, and you also escape the automatic int->bool conversion purgatory

(In reply to comment #2) > (From update of attachment 140849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140849&action=review > > In general this patch looks fine (aside from the bool vs. enum issue i mention), but i'm somewhat tired so don't feel sufficiently focussed to r+ it. Will re-review in the morning. > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:114 > > - VariableAccessData* newVariableAccessData(int operand) > > + VariableAccessData* newVariableAccessData(int operand, bool isCaptured) > > We try to use enums rather than bools in cases like this as they're more descriptive, and you also escape the automatic int->bool conversion purgatory I agree that in general that is the right way to do it. But here a bool is really a lot better. Consider what would happen if this was an enum. The most common idiom for this method is to say: bool isCaptured = something->isCaptured(); ... // bunch of code if (isCaptured) { ... /* do special things */ ... } ... // a lot of other code stuff = newVariableAccessData(thingy, isCaptured); If newVariableAccessData() took an enum instead, then I'd either have to make isCaptured() return that enum, or I'd have to make this code absolutely horrible: stuff = newVariableAccessData(thingy, isCaptured ? Captured : NotCaptured); That is clearly a regression. The alternative is to have isCaptured() and mergeIsCaptured() use the enum, but then I'd lose the clarify of using boolean arithmetic. Notice that mergeIsCaptured() does things like: bool newIsCaptured = m_isCaptured | isCaptured Do you really want this to become: CaptureMode newIsCaptured = ((m_isCaptured == Captured) | (isCaptured == Captured)) ? Captured : NotCaptured; Finally, there is only *one* place where newVariableAccessData() is called in a manner that leads the second argument to be confusing (i.e. where we pass false as the second argument). In all other places we pass a variable called "isCaptured" as the second argument. So, I just don't buy that turning this into an enum is going to make anyone's life any easier.

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

Merged in http://trac.webkit.org/changeset/118136