dart2js: simplify constant expression generation.

dart2js: simplify constant expression generation.

R=johnniwinther@google.com

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

[floitsch, 2015-02-24 23:08:48]

floitsch@google.com changed reviewers:
+ herhut@google.com, johnniwinther@google.com

[floitsch, 2015-02-24 23:08:49]

I tested on all language tests and the output is identical.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (left):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:297:
List<jsAst.Expression> elements = _array(constant.entries);
I initially thought that I could get away with an Iterable here and removed the
"_array" function (which had a bad name anyway).
It turns out that I couldn't and the function is now inlined in two places. I
think that's ok, but I can revive the function again (with name "referenceAll"
?).

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:24: final Function
constantReference;
I know the function isn't typed, but I really hate all these typedefs. In this
case the function is checked in the constructor.

If I'm overruled I can add a typedef, though...

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
moved here to live with the other constant-related functions.

I think we should rename this function to "referenceConstant". It either gives a
reference or the inlined constant.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:131: bool
isConstantInlinedOrAlreadyEmitted(ConstantValue constant) {
just copied from below.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:143: int
compareConstants(ConstantValue a, ConstantValue b) {
just copied from below.

[Johnni Winther, 2015-02-25 11:37:02]

lgtm

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:24: final Function
constantReference;
Make a typedef like

typedef jsAst.Expression ConstantReferenceGenerator(ConstantValue constant);

and rename the field to

ConstantReferenceGenerator generateConstantReference;

The latter will make the call-sites read nicer.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:28: * The given
[constantReferenc] function must, when invoked with a constant,
[constantReferenc] -> [constantReference]  (if not superceeded by the effect of
the previous comment)

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
On 2015/02/24 23:08:48, floitsch wrote:
> moved here to live with the other constant-related functions.
> 
> I think we should rename this function to "referenceConstant". It either gives
a
> reference or the inlined constant.

Maybe 'generateConstantReference'.

[floitsch, 2015-02-25 16:35:18]

One question still undecided.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:24: final Function
constantReference;
On 2015/02/25 11:37:02, Johnni Winther wrote:
> Make a typedef like
> 
> typedef jsAst.Expression ConstantReferenceGenerator(ConstantValue constant);
> 
> and rename the field to
> 
> ConstantReferenceGenerator generateConstantReference;
> 
> The latter will make the call-sites read nicer.
> 

Done.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:28: * The given
[constantReferenc] function must, when invoked with a constant,
On 2015/02/25 11:37:02, Johnni Winther wrote:
> [constantReferenc] -> [constantReference]  (if not superceeded by the effect
of
> the previous comment)

Done.

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
On 2015/02/25 11:37:02, Johnni Winther wrote:
> On 2015/02/24 23:08:48, floitsch wrote:
> > moved here to live with the other constant-related functions.
> > 
> > I think we should rename this function to "referenceConstant". It either
gives
> a
> > reference or the inlined constant.
> 
> Maybe 'generateConstantReference'.

The thing is, that it doesn't always generate a reference, but frequently just
returns the literal. That's why "referenceConstant" feel better to me. "I'm
referencing a constant. I can get back the value or a way to read it".

[Johnni Winther, 2015-02-26 08:24:50]

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
On 2015/02/25 16:35:18, floitsch wrote:
> On 2015/02/25 11:37:02, Johnni Winther wrote:
> > On 2015/02/24 23:08:48, floitsch wrote:
> > > moved here to live with the other constant-related functions.
> > > 
> > > I think we should rename this function to "referenceConstant". It either
> gives
> > a
> > > reference or the inlined constant.
> > 
> > Maybe 'generateConstantReference'.
> 
> The thing is, that it doesn't always generate a reference, but frequently just
> returns the literal. That's why "referenceConstant" feel better to me. "I'm
> referencing a constant. I can get back the value or a way to read it".

I rarely read 'reference' as a verb...

[floitsch, 2015-02-26 13:11:33]

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
On 2015/02/26 08:24:50, Johnni Winther wrote:
> On 2015/02/25 16:35:18, floitsch wrote:
> > On 2015/02/25 11:37:02, Johnni Winther wrote:
> > > On 2015/02/24 23:08:48, floitsch wrote:
> > > > moved here to live with the other constant-related functions.
> > > > 
> > > > I think we should rename this function to "referenceConstant". It either
> > gives
> > > a
> > > > reference or the inlined constant.
> > > 
> > > Maybe 'generateConstantReference'.
> > 
> > The thing is, that it doesn't always generate a reference, but frequently
just
> > returns the literal. That's why "referenceConstant" feel better to me. "I'm
> > referencing a constant. I can get back the value or a way to read it".
> 
> I rarely read 'reference' as a verb...

useConstant?

[Johnni Winther, 2015-02-26 15:01:46]

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
constantReference(ConstantValue constant);
On 2015/02/26 13:11:33, floitsch wrote:
> On 2015/02/26 08:24:50, Johnni Winther wrote:
> > On 2015/02/25 16:35:18, floitsch wrote:
> > > On 2015/02/25 11:37:02, Johnni Winther wrote:
> > > > On 2015/02/24 23:08:48, floitsch wrote:
> > > > > moved here to live with the other constant-related functions.
> > > > > 
> > > > > I think we should rename this function to "referenceConstant". It
either
> > > gives
> > > > a
> > > > > reference or the inlined constant.
> > > > 
> > > > Maybe 'generateConstantReference'.
> > > 
> > > The thing is, that it doesn't always generate a reference, but frequently
> just
> > > returns the literal. That's why "referenceConstant" feel better to me.
"I'm
> > > referencing a constant. I can get back the value or a way to read it".
> > 
> > I rarely read 'reference' as a verb...
> 
> useConstant?

It feels wrong; like registering that we use the constant. I'll prefer
'referenceConstant' then.

[floitsch, 2015-02-27 12:34:41]

On 2015/02/26 15:01:46, Johnni Winther wrote:
>
https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
> File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):
> 
>
https://codereview.chromium.org/947333004/diff/1/pkg/compiler/lib/src/js_emit...
> pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:448: jsAst.Expression
> constantReference(ConstantValue constant);
> On 2015/02/26 13:11:33, floitsch wrote:
> > On 2015/02/26 08:24:50, Johnni Winther wrote:
> > > On 2015/02/25 16:35:18, floitsch wrote:
> > > > On 2015/02/25 11:37:02, Johnni Winther wrote:
> > > > > On 2015/02/24 23:08:48, floitsch wrote:
> > > > > > moved here to live with the other constant-related functions.
> > > > > > 
> > > > > > I think we should rename this function to "referenceConstant". It
> either
> > > > gives
> > > > > a
> > > > > > reference or the inlined constant.
> > > > > 
> > > > > Maybe 'generateConstantReference'.
> > > > 
> > > > The thing is, that it doesn't always generate a reference, but
frequently
> > just
> > > > returns the literal. That's why "referenceConstant" feel better to me.
> "I'm
> > > > referencing a constant. I can get back the value or a way to read it".
> > > 
> > > I rarely read 'reference' as a verb...
> > 
> > useConstant?
> 
> It feels wrong; like registering that we use the constant. I'll prefer
> 'referenceConstant' then.

I will keep the constantReference then...

[floitsch, 2015-02-27 12:35:12]

Committed patchset #3 (id:40001) manually as 44089 (presubmit successful).