1st round of comments...
https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h#newcode351 src/hydrogen-instructions.h:351: return IsInteger32() ? Integer32() : Tagged(); I think we should add an assertion here about the non-Integer32 case (it has to be Tagged, i guess). I think we rely on the fact that things get monotonically more specialized. https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h#newcode2988 src/hydrogen-instructions.h:2988: ? Representation::Tagged() : Formatting nit: Consistently indent the ternary operator like condition ? trueValue : falseValue if it doesn't fit into a line. I know it's not done consistently in our code, but we shouldn't make it worse. :-) Alternatively, one might consider using a switch, which is even more readable IMHO. https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h#newcode2989 src/hydrogen-instructions.h:2989: (index == 3 ? Representation::None() : Hmmm, I would have expected checked_index()->representation().KeyedAccessIndexRequirement() instead of Representation::None(). If this is really intended, please add a comment why this is different, it is not obvious when reading the CL. https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h#newcode4845 src/hydrogen-instructions.h:4845: (index == 3 ? Representation::None() : Representation::Tagged()); See comments regarding indentation/adding a comment above. https://codereview.chromium.org/11445016/diff/6001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/11445016/diff/6001/src/hydrogen.cc#newcode3659 src/hydrogen.cc:3659: if (!FLAG_array_bounds_checks_elimination) return; For consistency with the other phases, move this to the call site. https://codereview.chromium.org/11445016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
