Accept constant string concatenation and constant string length as constant

Accept constant string concatenation and constant string length as constant
expressions (fix issue 22408).

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43831

[regis, 2015-02-17 22:02:03]

regis@google.com changed reviewers:
+ hausner@google.com

[regis, 2015-02-17 22:02:04]

Matthias,

I am not sure I am doing the right thing with IsPotentiallyConst().

In case you happen to have a comment explaining its purpose, I'll gladly add it
to ast.h in my cl :-)

Thanks,
Regis

[hausner, 2015-02-17 23:07:03]

IsPotentiallyConst looks ok to me.

IsPotentiallyConst is an attempt to detect expressions that are known not to be
compile time constants. The language spec has this concept of a "potentially
constant expression", which is an expression that is a compile time constant
given certain actual values. This is used in initializer lists of const
constructors. E.g., an initializer a*10 is a compile time constant if a is a
number, but not if a is a string or a List, for example.

When we compile a const constructor, we can sometimes detect whether an
expression is known NOT to be const in any case (i.e. IsPotentiallyConst returns
false). If so, the compiler reports an error. If not, then we fail at runtime if
the resulting expression is not a number or a boolean or a string, I think. We
still have bugs in detecting potentially constant expressions. I think we are
too permissive.

Your change LGTM (w/comments) if/when the expression s.length + 1 works.

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc#newcode548
runtime/vm/ast.cc:548: field_name().Equals(Symbols::Length())) {
Maybe reverse the order of checks? Testing whether the field name is identical
to the length symbol is cheap and rules out all non-const cases right away.
Ditto in the case above.

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc#newcode549
runtime/vm/ast.cc:549: return receiver_val;
I think you should return a value that represents an integer, not a string.
Otherwise the expression str.length + 1 will not evaluate to a compile time
constant. I think you can just return Integer::Handle() or
Smi::Handle(Smi::New(0)).

I would also add the above case to the test.

[regis, 2015-02-18 00:37:56]

New patchsets have been uploaded after l-g-t-m from hausner@google.com

[regis, 2015-02-18 00:38:08]

Thanks for the review and the explanation. I've added a comment summarizing (I
hope) the idea behind IsPotentiallyConst.

-- Regis

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc#newcode548
runtime/vm/ast.cc:548: field_name().Equals(Symbols::Length())) {
On 2015/02/17 23:07:03, hausner wrote:
> Maybe reverse the order of checks? Testing whether the field name is identical
> to the length symbol is cheap and rules out all non-const cases right away.
> Ditto in the case above.

Done.

https://codereview.chromium.org/935713002/diff/1/runtime/vm/ast.cc#newcode549
runtime/vm/ast.cc:549: return receiver_val;
On 2015/02/17 23:07:03, hausner wrote:
> I think you should return a value that represents an integer, not a string.
> Otherwise the expression str.length + 1 will not evaluate to a compile time
> constant. I think you can just return Integer::Handle() or
> Smi::Handle(Smi::New(0)).
> 
> I would also add the above case to the test.

Good catch. length + 1 was not working. It does now.

[regis, 2015-02-18 00:38:48]

Committed patchset #2 (id:20001) manually as r43831 (presubmit successful).