Consolidated the runtime calls to a single runtime call and split the Call class into Call and CallEval. This obsoleted most of the comments :). The rest are fixed.
Uploaded at http://codereview.chromium.org/12673 On Wed, Nov 26, 2008 at 1:39 PM, Mads Ager <[EMAIL PROTECTED]> wrote: > OK, I just tested evaluation order in a webkit nightly and in firefox. > Apparently, they are also treating eval in a special way, so the > evaluation order of your new code seems to be what they are doing for > calls to something that might be eval. > > eval(eval('var eval = function f() { print("hep"); }')) > > prints hep in our current implementation but does not in Firefox and > Safari. So the change in evaluation order seems to be a feature. :-) > > > On Wed, Nov 26, 2008 at 1:12 PM, <[EMAIL PROTECTED]> wrote: > > I have a couple of comments on top of Kasper's. I think we need to be > > careful with evaluation order here. > > > > > > http://codereview.chromium.org/12673/diff/1/15 > > File src/codegen-arm.cc (right): > > > > http://codereview.chromium.org/12673/diff/1/15#newcode2323 > > Line 2323: Label after_call; > > Move this new code before the "Fast-case" comment and comment on what it > > is doing. > > > > http://codereview.chromium.org/12673/diff/1/15#newcode2328 > > Line 2328: Load(function); > > Does this give us the correct evaluation order? Shouldn't we always > > load the arguments first and then load the function? Loading the > > arguments might change what the function should resolve to. > > > > http://codereview.chromium.org/12673/diff/1/15#newcode2331 > > Line 2331: __ b(eq, ¬_direct_eval); > > Is it OK to branch out here and redo the function call? You have > > already evaluated the function expression, and falling into the next > > code, you will do that again? Since evaluating the function expression > > might have side-effects we shouldn't do that. > > > > We should probably handle the call in this code no matter if it is an > > eval call or not. > > > > http://codereview.chromium.org/12673/diff/1/6 > > File src/macro-assembler-arm.h (right): > > > > http://codereview.chromium.org/12673/diff/1/6#newcode191 > > Line 191: // Takes the holder as an object pointer, finds its instance > > type > > Takes the register that holds the holder object pointer ... > > > > http://codereview.chromium.org/12673 > > > -- -- Ole I Hougaard Google Aarhus, Denmark +45 8745 9215 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
