Bug 99557

Created attachment 169110 [details] possibly the patch

Comment on attachment 169110 [details] possibly the patch Attachment 169110 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14397471

Comment on attachment 169110 [details] possibly the patch Attachment 169110 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14386476

Comment on attachment 169110 [details] possibly the patch Attachment 169110 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14393501

Comment on attachment 169110 [details] possibly the patch Attachment 169110 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14398469

Created attachment 169125 [details] the patch Performance neutral, appears to pass things. Now it even works on 32-bit, too.

Comment on attachment 169125 [details] the patch Nah, spoke too soon. Will investigate.

Created attachment 169136 [details] the patch I think it's good now.

Comment on attachment 169136 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review r=me > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438 > + // This passes NoExpectedFunction because we expect that if the function is in a > + // local variable, then it's not one of our built-in constructors. Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors". > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456 > + // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction. Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader.

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

(In reply to comment #9) > (From update of attachment 169136 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review > > r=me > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438 > > + // This passes NoExpectedFunction because we expect that if the function is in a > > + // local variable, then it's not one of our built-in constructors. > > Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors". I actually think the comment is more accurate. Just because we resolved to a non-local variable called "Object" doesn't mean we proved that we have the Object constructor. Likewise, just because resolved to a local variable called "Object" doesn't mean we disproved that we have the Object constructor because people could do crazy: var x = Object; function foo() { var Object = x; return new Object(); } It's just a matter of likelihood. The above snippet is less likely than someone just using the word "Object" to signify a local variable, or at least, that's what I'm assuming in this code path. > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456 > > + // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction. > > Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader. Actually this comment is just plain wrong. I've removed the comment.

(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 169136 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review > > > > r=me > > > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438 > > > + // This passes NoExpectedFunction because we expect that if the function is in a > > > + // local variable, then it's not one of our built-in constructors. > > > > Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors". > > I actually think the comment is more accurate. Just because we resolved to a non-local variable called "Object" doesn't mean we proved that we have the Object constructor. Likewise, just because resolved to a local variable called "Object" doesn't mean we disproved that we have the Object constructor because people could do crazy: > > var x = Object; > function foo() { > var Object = x; > return new Object(); > } > > It's just a matter of likelihood. The above snippet is less likely than someone just using the word "Object" to signify a local variable, or at least, that's what I'm assuming in this code path. > > > > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456 > > > + // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction. > > > > Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader. > > Actually this comment is just plain wrong. I've removed the comment. Removed the comment in http://trac.webkit.org/changeset/131646