Random superficial comments below. I'll take a closer look tonight or in the morning.
http://codereview.chromium.org/57052/diff/40/44 File src/codegen-ia32.h (right): http://codereview.chromium.org/57052/diff/40/44#newcode435 Line 435: void GenericBinaryOperation(Token::Value op, While you're at it, fix this indentation so Token::Value op is on the next line and aligned with the other arguments. http://codereview.chromium.org/57052/diff/40/49 File src/register-allocator.h (right): http://codereview.chromium.org/57052/diff/40/49#newcode47 Line 47: inline StaticType() : static_type_(UNKNOWN_TYPE) {} We don't usually have such dense blocks of declarations in classes in the V8 code. You might add some blank lines between blocks to break them up. I think it's better (easier to read) to leave inline off when the definition is in the class. http://codereview.chromium.org/57052/diff/40/49#newcode56 Line 56: bool is_known() const { return static_type_ != UNKNOWN_TYPE; } Do we need is_known and (two copies of) is_unknown? I'd normally prefer just is_known (which can be negated for is_unknown), but since "unknown" is actually an element of the order, it might make sense to have only is_unknown. http://codereview.chromium.org/57052/diff/40/49#newcode58 Line 58: bool is_likely_smi() const { return static_type_ & LIKELY_SMI_TYPE != 0; } Isn't this (static_type_ & LIKELY_SMI_TYPE) != 0? http://codereview.chromium.org/57052/diff/40/49#newcode62 Line 62: return static_type_ & HEAP_OBJECT_TYPE != 0; Isn't this (static_type_ & HEAP_OBJECT_TYPE) != 0? http://codereview.chromium.org/57052 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
