I have addressed all mentioned issues. Please give it another look.
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. On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Which bits are first and last? Do you mean low and high/most or least signficant?
Done. http://codereview.chromium.org/619005/diff/6030/6036#newcode69 src/diy_fp.h:69: const uint64_t kM32 = 0xFFFFFFFF; On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Put a "u" on the end of the literal to make it unsigned.
Done. 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" On 2010/02/22 11:31:36, Lasse Reichstein wrote:
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. Done. http://codereview.chromium.org/619005/diff/6030/6037#newcode36 src/double.h:36: typedef union { On 2010/02/22 11:31:36, Lasse Reichstein wrote:
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. Done. http://codereview.chromium.org/619005/diff/6030/6037#newcode58 src/double.h:58: static const int kSignificandSize = 52; // excluding the hidden bit On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Format comments as full sentences (capital start letter, full stop at
end). Done. http://codereview.chromium.org/619005/diff/6030/6037#newcode64 src/double.h:64: DiyFp AsDiyFp() const { On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Do you have any suggested pronunciation of DiyFp? :)
Do It Yourself Floating Point. Name is copied from underlying paper. But I'm willing to change it. 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); On 2010/02/22 11:31:36, Lasse Reichstein wrote:
GRISU -> V8_2PART
Done. 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)) On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Put a "## u" after the b to make it an unsigned constant. Just to be
safe. Done. 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: On 2010/02/22 11:31:36, Lasse Reichstein wrote:
For readability, could you reverse the order of the cases in each
group, so they
come in decreasing order
Done. http://codereview.chromium.org/619005/diff/6030/6033#newcode447 src/grisu3.cc:447: int K; On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Lower case variable names (and preferably not single-letter names).
Done. 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 On 2010/02/22 11:31:36, Lasse Reichstein wrote:
Is this line correct (or should there be a .scm after the file name?).
Line is correct. The scm-file must be compiled first. I have added a comment in the scm file. http://codereview.chromium.org/619005/diff/6030/6038#newcode3 src/powers_ten.h:3: On 2010/02/22 11:31:36, Lasse Reichstein wrote:
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?)
Lints now. http://codereview.chromium.org/619005 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
