LGTM with comments. Are the existing unit-tests covering all the cases?
http://codereview.chromium.org/1374005/diff/6001/7003 File src/conversions.cc (right): http://codereview.chromium.org/1374005/diff/6001/7003#newcode277 src/conversions.cc:277: static bool inline isDigit(int x, int radix) { I don't think it's necessary to mark a function as 'inline'. The compiler is usually better at these things than we are. ymmv http://codereview.chromium.org/1374005/diff/6001/7003#newcode285 src/conversions.cc:285: template <int radix_log_2, class Iterator, class EndMark> Add comment that current must not be end. http://codereview.chromium.org/1374005/diff/6001/7003#newcode318 src/conversions.cc:318: number = (number << radix_log_2) | digit; number = number * radix + digit. The compiler will be smart enough to change that into a shifts and pluses. http://codereview.chromium.org/1374005/diff/6001/7003#newcode335 src/conversions.cc:335: if (dropped_bits >= middle_value) { Since half-way cases do not always round down (see my next comment) I suggest splitting into three cases: if (dropped_bits < middle_value) { // round down } else if (dropped_bits > middle_value) { // round up } else { // dropped_bits == middle_value // wait for next digits } http://codereview.chromium.org/1374005/diff/6001/7003#newcode338 src/conversions.cc:338: // if the tail consists of zeros. I don't think this is the right approach. Generally we prefer rounding towards even. That is, if the last non-dropped digit is even, round down, otherwise round up. Otherwise we obtain different numbers when reading a number as decimal or as hexadecimal. http://codereview.chromium.org/1374005/diff/6001/7003#newcode339 src/conversions.cc:339: do { Since we need to run through the remaining digits anyways, it would probably make sense to determine the final rounding direction in the next loop. The 'if'-case would be smaller, and I don't think it would add too much in the next loop. http://codereview.chromium.org/1374005/diff/6001/7003#newcode352 src/conversions.cc:352: } else { // driopped_bit > middle_value dropped http://codereview.chromium.org/1374005/diff/6001/7003#newcode396 src/conversions.cc:396: // and sign. Assuming it's a rare case more simple code is used. You could use/extend double.h. It already contains the reading part. I don't think adding the writing-part would be difficult. I expect 'pow' to be optimized for '2', and the given approach is fine. (Otherwise someone would need to deal with infinity...) http://codereview.chromium.org/1374005/diff/6001/7003#newcode397 src/conversions.cc:397: printf("Exponent: %d (%f))\n", exponent, pow(2.0, exponent)); remove printf. http://codereview.chromium.org/1374005 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
