Allow numeric literals to be checked for a decimal point.

Allow numeric literals to be checked for a decimal point.

The asm.js spec decides the type of numeric literals in several places
based on if they contain a ".".
http://asmjs.org/spec/latest/

Adding methods so that AST Literals can be checked for containg a dot.

Adding a cctest that this information is available.

LOG=N
BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=test-parsing
R=rossberg@chromium.org,titzer@chromium.org

Committed: https://crrev.com/9adb5f0a59210a3c46eb17ed00226d2461192af4
Cr-Commit-Position: refs/heads/master@{#29395}

[bradn, 2015-06-21 21:41:53]

Two questions:
* Should I plumb through the dot detection deeper into StringToDouble. I'm
unsure how much to lean on the compiler to do good things?
* Is there a more compact pattern to test this with?

[titzer, 2015-06-22 07:08:16]

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1438
src/scanner.cc:1438: Vector<const uint8_t> literal_string =
literal_one_byte_string();
Does this make a copy of the bytes underneath?

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1441
src/scanner.cc:1441: with_dot |= (literal_string[i] == '.');
Early return?

[Sven Panne, 2015-06-22 07:53:42]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2015-06-22 07:53:43]

DBC

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1436
src/scanner.cc:1436: bool Scanner::ContainsDot() {
Instead of using caveman-style loops, the right way to implement this is to make
Scanner/LiteralBuffer obey the iterator protocol. With that, everything is more
flexible and ContainsDot would be a one-liner with std::find, so it's probably
not even worth having as a separate function.

[titzer, 2015-06-22 07:59:35]

On 2015/06/22 07:53:43, Sven Panne wrote:
> DBC
> 
> https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc
> File src/scanner.cc (right):
> 
> https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1436
> src/scanner.cc:1436: bool Scanner::ContainsDot() {
> Instead of using caveman-style loops, the right way to implement this is to
make
> Scanner/LiteralBuffer obey the iterator protocol. With that, everything is
more
> flexible and ContainsDot would be a one-liner with std::find, so it's probably
> not even worth having as a separate function.

That sounds like a refactoring that would make sense in a different CL, separate
from the functionality introduced here.

[Sven Panne, 2015-06-22 08:03:45]

On 2015/06/22 07:59:35, titzer wrote:
> That sounds like a refactoring that would make sense in a different CL,
separate
> from the functionality introduced here.

Yep, see http://martinfowler.com/articles/workflowsOfRefactoring/#2hats  :-)
Alas, we normally don't follow that general advice...

[bradn, 2015-06-23 06:54:07]

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1436
src/scanner.cc:1436: bool Scanner::ContainsDot() {
On 2015/06/22 07:53:43, Sven Panne wrote:
> Instead of using caveman-style loops, the right way to implement this is to
make
> Scanner/LiteralBuffer obey the iterator protocol. With that, everything is
more
> flexible and ContainsDot would be a one-liner with std::find, so it's probably
> not even worth having as a separate function.

Turns out Vector<> provides begin/end, which allow some greater brevity.

LiteralBuffer manages providing subranges (in multiple byte sizes), which it
returns as Vector<>.

I suppose it could provide these via two separate methods, but these in turn
would have to be carried through two layers of wrapper out to Scanner. (The
convience methods in Scanner are presumably due to the number of uses, i.e.
many).

Having this as a method allows the DCHECK (which judging by the underlying
implementation seems to be allowed to be null under some circumstances?)
It might be worth incorporating into DoubleValue(), but see below.

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1438
src/scanner.cc:1438: Vector<const uint8_t> literal_string =
literal_one_byte_string();
On 2015/06/22 07:08:16, titzer wrote:
> Does this make a copy of the bytes underneath?

No, it copies a reference to the underying buffer and size (And is a weak
reference in contrast to ScopedVector).

When I'd asked about leaning on the compiler though, what I'd meant was how
frugal about copies of Vector<>? (The other functions here, DoubleValue for
instance seem to assume instantiating a Vector<> (costing 2 pointers, times 2
layers of return), is an acceptable cost, presumably hoping for optimization.)

DoubleValue() is only used twice, once needing to know about dots, the other
not. So I could add a bool* with_dot parameter to it, but that struck me as odd.

Trying to calibrate my micro-optimization instincts for this code base. Sorry
coming from NaCl core, which is filled with C code (for instance, we're
literally having a should we allow any C++ in corner X of the codebase
discussion next week :-).

https://codereview.chromium.org/1201783003/diff/1/src/scanner.cc#newcode1441
src/scanner.cc:1441: with_dot |= (literal_string[i] == '.');
On 2015/06/22 07:08:16, titzer wrote:
> Early return?

Replaced.
FWIW was lifted wholesale from a prototype CL Andreas pointed me at :-)

https://codereview.chromium.org/1201783003/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1201783003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:1105: v8::V8::Initialize();
Any pointers on how to reduce this?
Speaking of refactoring, I'm tempted to roll this boilerplate setup into
something common, shared at least with the one above? Though I've heard testing
nerds suggest hard to setup objects are code smell of sorts.

[bradn, 2015-06-29 20:47:55]

ping?

[titzer, 2015-06-30 07:51:10]

lgtm

[rossberg, 2015-06-30 12:07:01]

LGTM, mostly nits.

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:148: bool ContainsDot() const { return type_ ==
NUMBER_WITH_DOT; }
Nit: IsFloating perhaps?

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:160: return smi_;
Nit: keep layout consistent

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:193: NUMBER_WITH_DOT,
Nit: can we call this FLOATING?

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:218: void MarkWithDot() {
Get rid of this method and add an is_float flag parameter to the
AstValue(double) constructor instead.

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:344: const AstValue* NewNumber(double number, bool
with_dot = false);
Nit: is_float?

[titzer, 2015-06-30 12:22:41]

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:148: bool ContainsDot() const { return type_ ==
NUMBER_WITH_DOT; }
On 2015/06/30 12:07:01, rossberg wrote:
> Nit: IsFloating perhaps?

I'd actually prefer to keep it the way it is, because floating point numbers can
also take the form "1e-5", which doesn't contain a dot, unless you are
suggesting the latter should also be recognized.

[rossberg, 2015-06-30 12:29:32]

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:148: bool ContainsDot() const { return type_ ==
NUMBER_WITH_DOT; }
On 2015/06/30 12:22:41, titzer wrote:
> On 2015/06/30 12:07:01, rossberg wrote:
> > Nit: IsFloating perhaps?
> 
> I'd actually prefer to keep it the way it is, because floating point numbers
can
> also take the form "1e-5", which doesn't contain a dot, unless you are
> suggesting the latter should also be recognized.

OK, fair enough.

[bradn, 2015-06-30 15:54:17]

FWIW, the spec has two main places this comes up (in the positive), near
annotations and later with numeric literals. In the annotations section  only
'.'s are mentioned. Near literals case has the ambiguous phrase: "or validates
as type double".
I doubt in practice actual asm.js code cares, so I would say we could push back
for an edit either way round? Both are equally expressive. Allowing e as a
marker is slightly smaller for some constants.

What do you guys think?

ANNOTATIONS:
In the first case, the variable type is double if n's source contains the
character .; otherwise n may be an integer literal in the range [-231, 232), in
which case the variable type is int.

LITERALS:
succeeds with type double if the source contains a . character; or validates as
type double;

[rossberg, 2015-06-30 16:26:32]

On 2015/06/30 15:54:17, bradn wrote:
> FWIW, the spec has two main places this comes up (in the positive), near
> annotations and later with numeric literals. In the annotations section  only
> '.'s are mentioned. Near literals case has the ambiguous phrase: "or validates
> as type double".

Yes, I found this half sentence rather ill-defined. The spec doesn't ever say
what that means. Probably an editing bug.

> I doubt in practice actual asm.js code cares, so I would say we could push
back
> for an edit either way round? Both are equally expressive. Allowing e as a
> marker is slightly smaller for some constants.
> 
> What do you guys think?

I'm fine either way, but with a slight preference for sticking to the spec
(which supposedly requires a point).

[bradn, 2015-06-30 16:58:15]

We clarified with Alon Zakai, it sounds like '.' is the sole marker, as 1e3 for
ints etc. are currently done.

[bradn, 2015-06-30 20:03:47]

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:148: bool ContainsDot() const { return type_ ==
NUMBER_WITH_DOT; }
On 2015/06/30 12:07:01, rossberg wrote:
> Nit: IsFloating perhaps?

Left as is as per discussion.

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:160: return smi_;
On 2015/06/30 12:07:01, rossberg wrote:
> Nit: keep layout consistent

Done.

(Was a product of the git cl format autoformatter FWIW).

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:193: NUMBER_WITH_DOT,
On 2015/06/30 12:07:01, rossberg wrote:
> Nit: can we call this FLOATING?

Sticking with WITH_DOT as per discussion.

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:218: void MarkWithDot() {
On 2015/06/30 12:07:01, rossberg wrote:
> Get rid of this method and add an is_float flag parameter to the
> AstValue(double) constructor instead.

Done, but with with_dot as per discussion.

https://codereview.chromium.org/1201783003/diff/40001/src/ast-value-factory.h...
src/ast-value-factory.h:344: const AstValue* NewNumber(double number, bool
with_dot = false);
On 2015/06/30 12:07:01, rossberg wrote:
> Nit: is_float?

Sticking with with_dot as per discussion.

[bradn, 2015-06-30 20:03:53]

The CQ bit was checked by bradnelson@google.com

[bradn, 2015-06-30 20:03:53]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1201783003/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2015-06-30 20:04:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1201783003/60001

[commit-bot: I haz the power, 2015-06-30 21:12:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-30 21:12:32]

Patchset 4 (id:??) landed as
https://crrev.com/9adb5f0a59210a3c46eb17ed00226d2461192af4
Cr-Commit-Position: refs/heads/master@{#29395}