super and this in compile-time constants.

super and this in compile-time constants.

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

[ahe, 2012-03-23 10:56:15]

https://chromiumcodereview.appspot.com/9753001/diff/4002/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/4002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:946: Function compileConstant = (x) =>
compiler.compileVariable(x);
What is up with this? Is there a VM bug? I have noticed that I had to do the
same when calling initializerDo.

[floitsch, 2012-03-23 23:15:57]

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:946: Function compileArgument = evaluate;
The VM has/had a bug here (see previous uploads), but Regis is working on a fix
right now. Normally he should commit before me.

[ahe, 2012-03-24 07:30:42]

DBC

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
File tests/language/src/CompileTimeConstantITest.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
tests/language/src/CompileTimeConstantITest.dart:33: final a1 = const A(499);
How about adding tests like this:

Expect.identical(a1, const A(499));
etc.

[ngeoffray, 2012-03-26 07:38:30]

Looks good, but please consider adding this ConstructorEvaluator abstraction, I
believe this will make the code a lot easier to follow.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:684:
CompileTimeConstantEvaluator.insideConstructor(this.constantHandler,
Because you can fetch constantHandler from the compiler, I would not pass it as
a parameter.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:940: if (send.arguments.isEmpty() &&
parameters.parameterCount == 0) {
Is that premature optimization? I would really get rid of this shortcut, it
should be handled by addSendArgumentsToList.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:955: * Given the arguments (a list of
constants) assigns the to the parameters,
the to the -> ?

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:960: FunctionElement constructor,
List<Constant> arguments,
one argument per line please

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1049: * constructors.
Having to also check it here is quite annoying. We're not checking it in the
codegen, so I don't think we should check it here either. The resolver should
handle it.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1078: Map<Element, Constant> fieldValues) {
I think it would be easier to read if this function was not local.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1112: Map<Element, Constant> fieldValues =
new Map<Element, Constant>();
Would it make sense to have an abstraction for a constructor evaluating itself?
Something whose API would look like:

ConstructorEvaluator evaluator = new ConstructorEvaluator(constructor,
compiler);

evaluator.evaluate(node.send); // or evaluator.evaluate(constants)

The ConstructorEvaluator class would contain the map of fieldValues, that this
method just needs to fetch.

Also, you would create a ConstructorEvaluator for each this redirect or super
call.

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
File tests/language/src/CompileTimeConstantITest.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
tests/language/src/CompileTimeConstantITest.dart:3: // BSD-style license that
can be found in the LICENSE file.
Great test!

[ahe, 2012-03-26 08:03:25]

DBC

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:940: if (send.arguments.isEmpty() &&
parameters.parameterCount == 0) {
On 2012/03/26 07:38:31, ngeoffray wrote:
> Is that premature optimization? I would really get rid of this shortcut, it
> should be handled by addSendArgumentsToList.

Note that these kind of hacks are not necessary if you use Link. It is designed
to work well in exactly these cases.

[floitsch, 2012-03-27 00:52:50]

PTAL.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:684:
CompileTimeConstantEvaluator.insideConstructor(this.constantHandler,
On 2012/03/26 07:38:31, ngeoffray wrote:
> Because you can fetch constantHandler from the compiler, I would not pass it
as
> a parameter.

Done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:940: if (send.arguments.isEmpty() &&
parameters.parameterCount == 0) {
On 2012/03/26 07:38:31, ngeoffray wrote:
> Is that premature optimization? I would really get rid of this shortcut, it
> should be handled by addSendArgumentsToList.

Done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:946: Function compileArgument = evaluate;
On 2012/03/23 23:15:57, floitsch wrote:
> The VM has/had a bug here (see previous uploads), but Regis is working on a
fix
> right now. Normally he should commit before me.
Fix has been committed.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:955: * Given the arguments (a list of
constants) assigns the to the parameters,
On 2012/03/26 07:38:31, ngeoffray wrote:
> the to the -> ?

Done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:960: FunctionElement constructor,
List<Constant> arguments,
On 2012/03/26 07:38:31, ngeoffray wrote:
> one argument per line please

Done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1049: * constructors.
On 2012/03/26 07:38:31, ngeoffray wrote:
> Having to also check it here is quite annoying. We're not checking it in the
> codegen, so I don't think we should check it here either. The resolver should
> handle it.

You are right. This is already handled by the resolver. Great.
done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1078: Map<Element, Constant> fieldValues) {
On 2012/03/26 07:38:31, ngeoffray wrote:
> I think it would be easier to read if this function was not local.

Done.

https://chromiumcodereview.appspot.com/9753001/diff/6018/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:1112: Map<Element, Constant> fieldValues =
new Map<Element, Constant>();
On 2012/03/26 07:38:31, ngeoffray wrote:
> Would it make sense to have an abstraction for a constructor evaluating
itself?
> Something whose API would look like:
> 
> ConstructorEvaluator evaluator = new ConstructorEvaluator(constructor,
> compiler);
> 
> evaluator.evaluate(node.send); // or evaluator.evaluate(constants)
> 
> The ConstructorEvaluator class would contain the map of fieldValues, that this
> method just needs to fetch.
> 
> Also, you would create a ConstructorEvaluator for each this redirect or super
> call.

Made ConstructorEvaluator a subclass.

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
File tests/language/src/CompileTimeConstantITest.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/6018/tests/language/src/C...
tests/language/src/CompileTimeConstantITest.dart:33: final a1 = const A(499);
On 2012/03/24 07:30:42, ahe wrote:
> How about adding tests like this:
> 
> Expect.identical(a1, const A(499));
> etc.

I added some, but the identical-tests are mostly done in
CompileTimeConstantETest.

[ngeoffray, 2012-03-27 11:30:33]

LGTM!! Thanks for the cleanup.

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:681: Map<Element, Constant> get
definitions() => null;
Please consider removing these getters, and override visitSend in
ConstructorEvaluator. The visitSend in ConstructorEvaluator could look like:

Constant visitSend(Send send) {
  Element element = elements[send];
  if (!Elements.isLocal(element)) {
    return super.visitSend(send);
  }
  ...
}

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:929: List<Constant>
evaluateStaticSendArguments(Send send,
Since this is only used by the ConstructorEvaluator, you could move it there.

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:955: List<Constant> arguments =
evaluateStaticSendArguments(send, constructor);
evaluateStaticSendArguments -> evaluateArgumentsToConstructor ?

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:958:
evaluator.evaluateConstructorFieldValues(arguments);
Instead of passing arguments, I would pass the node and let CosntructorEvaluator
call evaluateStaticSendArguments.

[floitsch, 2012-03-28 00:10:58]

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:681: Map<Element, Constant> get
definitions() => null;
On 2012/03/27 11:30:33, ngeoffray wrote:
> Please consider removing these getters, and override visitSend in
> ConstructorEvaluator. The visitSend in ConstructorEvaluator could look like:
> 
> Constant visitSend(Send send) {
>   Element element = elements[send];
>   if (!Elements.isLocal(element)) {
>     return super.visitSend(send);
>   }
>   ...
> }

I was afraid that a local could be a Prefix too. But test confirmed that this is
not the case.
done.

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:929: List<Constant>
evaluateStaticSendArguments(Send send,
On 2012/03/27 11:30:33, ngeoffray wrote:
> Since this is only used by the ConstructorEvaluator, you could move it there.

It is actually used by the normal evaluator too (in visitNewExpression). I had
already moved it, but it doesn't really work, because the arguments need to be
evaluated in the context of the original expression.

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:955: List<Constant> arguments =
evaluateStaticSendArguments(send, constructor);
On 2012/03/27 11:30:33, ngeoffray wrote:
> evaluateStaticSendArguments -> evaluateArgumentsToConstructor ?

Done.

https://chromiumcodereview.appspot.com/9753001/diff/7004/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:958:
evaluator.evaluateConstructorFieldValues(arguments);
On 2012/03/27 11:30:33, ngeoffray wrote:
> Instead of passing arguments, I would pass the node and let
CosntructorEvaluator
> call evaluateStaticSendArguments.

Can't do that, because there is no Send for implicit super calls to the
default-constructor.