I glanced at this again. General comments: 1. It is still wrong. You did not address my previous comment: this optimization only correctly applies to addition.
2. You have refactored the switch statement in the loop and introduced a bug. Please do not do that. (Do not try to refactor code unless you understand it. Do not add new unrelated changes to a changelist *after* it has begun being reviewed, as the reviewer may miss them.) http://codereview.chromium.org/243051/diff/9/8003 File src/parser.cc (right): http://codereview.chromium.org/243051/diff/9/8003#newcode2810 Line 2810: double new_number = NAN; I find this new code less clear than the old because it's less direct. If you want to jump to the top of the loop ('continue'), why bother setting a variable, jumping to the bottom of the switch, and then checking the variable? http://codereview.chromium.org/243051/diff/9/8003#newcode2859 Line 2859: if (new_number != NAN) { You have introduced a bug. Because of the way NaN comparisons work, *everything* is not equal to NaN (even NaN != NaN). If you don't hit one of the cases in the switch you will constant-fold to NaN. 1==1 should be true, not NaN. http://codereview.chromium.org/243051/diff/9/8003#newcode2863 Line 2863: } else if (x_handle->IsString() && y_handle->IsString()) { No, you cannot do this optimization unless op is Token::ADD. http://codereview.chromium.org/243051/diff/9/8003#newcode2870 Line 2870: } else if (x && x->AsBinaryOperation() && y_handle->IsString()) { You cannot do this optimization unless op is Token::Add. http://codereview.chromium.org/243051 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
