Style and nitpick comments only.
http://codereview.chromium.org/619005/diff/6030/6036 File src/diy_fp.h (right): http://codereview.chromium.org/619005/diff/6030/6036#newcode68 src/diy_fp.h:68: // are only used for rounding the first 64 bits. Which bits are first and last? Do you mean low and high/most or least signficant? http://codereview.chromium.org/619005/diff/6030/6036#newcode69 src/diy_fp.h:69: const uint64_t kM32 = 0xFFFFFFFF; Put a "u" on the end of the literal to make it unsigned. http://codereview.chromium.org/619005/diff/6030/6037 File src/double.h (right): http://codereview.chromium.org/619005/diff/6030/6037#newcode31 src/double.h:31: #include "diy_fp.h" For consistency with the remaining code, please don't include .h files from .h files. Instead include the them both in any file that uses this .h file. http://codereview.chromium.org/619005/diff/6030/6037#newcode36 src/double.h:36: typedef union { Using a union to convert is bound to give problems with strict-aliasing (if we ever get to enabling that). Have you considered using the BitCast template instead? It uses memcpy to copy the bits, but is optimized very efficiently by compilers. http://codereview.chromium.org/619005/diff/6030/6037#newcode58 src/double.h:58: static const int kSignificandSize = 52; // excluding the hidden bit Format comments as full sentences (capital start letter, full stop at end). http://codereview.chromium.org/619005/diff/6030/6037#newcode64 src/double.h:64: DiyFp AsDiyFp() const { Do you have any suggested pronunciation of DiyFp? :) http://codereview.chromium.org/619005/diff/6030/6046 File src/globals.h (right): http://codereview.chromium.org/619005/diff/6030/6046#newcode103 src/globals.h:103: // write GRISU_UINT64_C(0x12345678,90123456); GRISU -> V8_2PART http://codereview.chromium.org/619005/diff/6030/6046#newcode104 src/globals.h:104: #define V8_2PART_UINT64_C(a, b) (((static_cast<uint64_t>(a) << 32) + 0x ## b)) Put a "## u" after the b to make it an unsigned constant. Just to be safe. http://codereview.chromium.org/619005/diff/6030/6033 File src/grisu3.cc (right): http://codereview.chromium.org/619005/diff/6030/6033#newcode167 src/grisu3.cc:167: case 32: For readability, could you reverse the order of the cases in each group, so they come in decreasing order http://codereview.chromium.org/619005/diff/6030/6033#newcode447 src/grisu3.cc:447: int K; Lower case variable names (and preferably not single-letter names). http://codereview.chromium.org/619005/diff/6030/6038 File src/powers_ten.h (right): http://codereview.chromium.org/619005/diff/6030/6038#newcode2 src/powers_ten.h:2: // command used: tools/generate-ten-powers --from -308 --to 342 --mantissa-size 64 --round round -o src/powers_ten.h Is this line correct (or should there be a .scm after the file name?). http://codereview.chromium.org/619005/diff/6030/6038#newcode3 src/powers_ten.h:3: Even if it's generated, do generate it with some newlines. It's going to blow someone's editor :) Also, does it lint? (Or how is it omitted from lint checking?) http://codereview.chromium.org/619005 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
