Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(197)

Issue 21509: Move inlined comparisons with null constant to use virtual frame constants. (Closed)

Created:
11 years, 10 months ago by William Hesse
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

Move inlined comparisons with null constant to use virtual frame constants. Committed: http://code.google.com/p/v8/source/detail?r=1317

Patch Set 1 #

Total comments: 4

Patch Set 2 : '' #

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -46 lines) Patch
M src/codegen-ia32.cc View 1 2 4 chunks +43 lines, -46 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
William Hesse
A small changelist.
11 years, 10 months ago (2009-02-19 12:42:58 UTC) #1
Kevin Millikin (Chromium)
LGTM. http://codereview.chromium.org/21509/diff/1/2 File src/codegen-ia32.cc (right): http://codereview.chromium.org/21509/diff/1/2#newcode1334 Line 1334: (left_side.is_constant() && left_side.handle()->IsNull() || This condition should ...
11 years, 10 months ago (2009-02-19 13:00:45 UTC) #2
William Hesse
11 years, 10 months ago (2009-02-19 13:20:50 UTC) #3
http://codereview.chromium.org/21509/diff/1/2
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21509/diff/1/2#newcode1334
Line 1334: (left_side.is_constant() && left_side.handle()->IsNull() ||
On 2009/02/19 13:00:45, Kevin Millikin wrote:
> This condition should be aligned with the previous one, cc == equal.

Done.

http://codereview.chromium.org/21509/diff/1/2#newcode1341
Line 1341: (left_side.is_constant() && left_side.handle()->IsNull()) ?
On 2009/02/19 13:00:45, Kevin Millikin wrote:
> I don't like breaking the ternary operator like this.  I'd rather see the ?
and
> : at the beginning of the following line.
> 
> You might consider just checking up front if left and right are null, at the
> same time you check for smi constants, ie:
> 
> bool left_is_constant_smi = false;
> bool left_is_constant_null = false;
> if (left_side.is_constant()) {
>   left_is_constant_smi = left_side.handle()->IsSmi();
>   left_is_constant_null = left_side.handle()->IsNull();
> }
> 
> Then eliminating the repeated && expression.

Done.

Powered by Google App Engine
This is Rietveld 408576698