Improve ScanJsonNumber.

Improve ScanJsonNumber.

[sandholm, 2011-02-04 13:37:34]

Simplified version...

[Lasse Reichstein, 2011-02-04 13:53:36]

LGTM with comments.

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc
File src/scanner.cc (right):

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc#newcode519
src/scanner.cc:519: bool sign = (c0_ == '-');
Call the variable "negative". 
It's not obvious what "sign" being true means.

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc#newcode521
src/scanner.cc:521: if (sign) AddLiteralCharAdvance();
I would prefer:
bool negative = false;
if (c0_ == '-') {
  negative = true;
  AddLiteralCharAdvance();
}

http://codereview.chromium.org/6334106/diff/1/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/1/src/scanner.h#newcode151
src/scanner.h:151: double number;
Make it a private (or protected) property and call it number_, then make a
function called number() that returns it.
This way, the number field is publicly writable.

[sandholm, 2011-02-04 14:26:36]

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc
File src/scanner.cc (right):

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc#newcode519
src/scanner.cc:519: bool sign = (c0_ == '-');
On 2011/02/04 13:53:36, Lasse Reichstein wrote:
> Call the variable "negative". 
> It's not obvious what "sign" being true means. 

Done.

http://codereview.chromium.org/6334106/diff/1/src/scanner.cc#newcode521
src/scanner.cc:521: if (sign) AddLiteralCharAdvance();
On 2011/02/04 13:53:36, Lasse Reichstein wrote:
> I would prefer:
> bool negative = false;
> if (c0_ == '-') {
>   negative = true;
>   AddLiteralCharAdvance();
> }

Done.

http://codereview.chromium.org/6334106/diff/1/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/1/src/scanner.h#newcode151
src/scanner.h:151: double number;
On 2011/02/04 13:53:36, Lasse Reichstein wrote:
> Make it a private (or protected) property and call it number_, then make a
> function called number() that returns it.
> This way, the number field is publicly writable.

Done.

[tfarina, 2011-02-04 14:46:48]

http://codereview.chromium.org/6334106/diff/2003/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/2003/src/scanner.h#newcode151
src/scanner.h:151: double number() {
double number() const { return number_; }

I'd add a comment here or in the member variable describing it. It's not clear
for what it serves.

[sandholm, 2011-02-04 14:57:22]

http://codereview.chromium.org/6334106/diff/2003/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/2003/src/scanner.h#newcode151
src/scanner.h:151: double number() {
On 2011/02/04 14:46:48, tfarina wrote:
> double number() const { return number_; }
> 
> I'd add a comment here or in the member variable describing it. It's not clear
> for what it serves.

Done.

[tfarina, 2011-02-04 15:04:44]

http://codereview.chromium.org/6334106/diff/8/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/8/src/scanner.h#newcode152
src/scanner.h:152: double number() {
I'll not insist in the const here, but I'd like to know if you tried compiling
with it and it didn't work. Also, I'll not insist in putting everything at the
same line. But it fits and it's very common practice on V8 code.

[Lasse Reichstein, 2011-02-07 08:40:43]

http://codereview.chromium.org/6334106/diff/8/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/6334106/diff/8/src/scanner.h#newcode152
src/scanner.h:152: double number() {
We generally don't use const in the V8 code.
It should work, and probably wouldn't hurt if added, but it's only necessary if
you ever have a const reference to the scanner, and we probably won't have that
either.

As for making it a one-liner ... I wouldn't insist either way either. I prefer
the three-line code, but I can understand the argument for having more
meaningful lines on screen at a atime.