Mainly formatting issues.
http://codereview.chromium.org/196139/diff/1/7 File src/objects.h (right): http://codereview.chromium.org/196139/diff/1/7#newcode908 Line 908: static inline bool IsValid(intptr_t value); Are there unit tests verifying the correct behavior on unsigned int? I assume an unsigned 32-bit int between 2^31 and 2^32 should not be a valid Smi. http://codereview.chromium.org/196139/diff/1/8 File src/utils.h (right): http://codereview.chromium.org/196139/diff/1/8#newcode39 Line 39: // Returns true iff x is a power of 2. Does not work for zero or max negative. Why doesn't this work for the minimum negative number? It looks to me like it does. http://codereview.chromium.org/196139/diff/1/14 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/196139/diff/1/14#newcode771 Line 771: __ SmiCompare(rcx, Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR)); If we had a SmiCompare(Operand, Smi), then we could optimize it in the 32-bit smi case to only read the top 32 bits, and compare directly with an immediate. http://codereview.chromium.org/196139/diff/1/16 File src/x64/macro-assembler-x64.cc (left): http://codereview.chromium.org/196139/diff/1/16#oldcode48 Line 48: We use two lines between function definitions in a .cc file. http://codereview.chromium.org/196139/diff/1/16 File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/196139/diff/1/16#newcode65 Line 65: Immediate(static_cast<int32_t> (~Page::kPageAlignmentMask))); What is this space here for? http://codereview.chromium.org/196139/diff/1/16#newcode243 Line 243: intptr_t p1 = reinterpret_cast<intptr_t> (msg); C++ style guide (for open-source, at googlecode.com) says no space between > and ( in a cast. http://codereview.chromium.org/196139/diff/1/16#newcode264 Line 264: ASSERT(allow_stub_calls()); // calls are not allowed in some stubs Need two spaces between ; and //. http://codereview.chromium.org/196139/diff/1/16#newcode1039 Line 1039: #else Comment this else, or else put a big header comment" ___ END OF 32-BIT SMI CODE, BEGINNING OF 31-BIT SMI CODE _______********** http://codereview.chromium.org/196139/diff/1/16#newcode1249 Line 1249: Register src1, Why aren't the arguments aligned with each other? I'm pretty sure that is the standard we use throughout V8. http://codereview.chromium.org/196139/diff/1/16#newcode1891 Line 1891: SizeOfCodeGeneratedSince(&target) + kPointerSize); C++ style guide says that function definitions, declarations, and calls are wrapped at the parenthesis, unless it is impossible to fit within the line length. http://codereview.chromium.org/196139/diff/1/16#newcode1948 Line 1948: push(Immediate(0)); // NULL frame pointer. This will not lint. Need two spaces before //. http://codereview.chromium.org/196139/diff/1/16#newcode1956 Line 1956: Put two blank lines between function definitions in a .cc file. http://codereview.chromium.org/196139/diff/1/16#newcode2741 Line 2741: } Don't change this to be different from all the other V8 files. http://codereview.chromium.org/196139/diff/1/21 File test/mjsunit/smi-negative-zero.js (right): http://codereview.chromium.org/196139/diff/1/21#newcode50 Line 50: assertEquals(-Infinity, one / (minus_four % two), "foo1"); If we are fixing this up, shouldn't we replace "foo1" with "one / (minus_four % two)"? http://codereview.chromium.org/196139 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
