Use an explicit 'this' parameter instead of 'This' nodes.

Use an explicit 'this' parameter instead of 'This' nodes.

An explicit parameter makes it possible to visit all uses of 'this', for example, to change to a calling convention where the receiver is passed as an explicit argument.

R=asgerf@google.com

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

[sra1, 2015-03-18 12:29:56]

sra@google.com changed reviewers:
+ asgerf@google.com

[sra1, 2015-03-18 12:31:12]

This is a prerequisite for methods that use the interceptor calling convention.

[asgerf, 2015-03-18 13:28:25]

Great! I've wanted to do the same thing for a while.

LGTM with comments.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:240: }
There is already a ThisLocal in closure.dart. Can we avoid having both?

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2058:
!Elements.isStaticOrTopLevel(state.currentElement)) {
I think this will also create a this parameter for functions nested inside a
static method (because the nested function is not considered static).

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2276: ir.Primitive value =
field.local is ThisLocal
I suspect this doesn't work anymore because of the new ThisLocal class.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:645: }
If the This class is not used anymore, then remove it.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:51: }
I think we try to avoid `visitXXX` names for methods that don't actually
override a visitor method.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:57: return
'$indentation(FunctionDefinition $name $thisName($parameters) return\n'
Long line

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:88: return
'$indentation(ConstructorDefinition $name$thisName($parameters) return'
Long line

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:454: return '<<${node}>>';
It's not clear if this is leftover debugging code or intentional. In any case,
there shouldn't be outcommented code.

[sra1, 2015-03-19 10:42:52]

PTAL.
I'm still working on a small number of tests.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:240: }
On 2015/03/18 13:28:25, asgerf wrote:
> There is already a ThisLocal in closure.dart. Can we avoid having both?

I kept this one (but renamed).
The other one has getEnclosingClass which we don't need.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2058:
!Elements.isStaticOrTopLevel(state.currentElement)) {
On 2015/03/18 13:28:25, asgerf wrote:
> I think this will also create a this parameter for functions nested inside a
> static method (because the nested function is not considered static).

Done.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2276: ir.Primitive value =
field.local is ThisLocal
On 2015/03/18 13:28:25, asgerf wrote:
> I suspect this doesn't work anymore because of the new ThisLocal class.

Good catch.  I just debugged that :-)

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:645: }
On 2015/03/18 13:28:25, asgerf wrote:
> If the This class is not used anymore, then remove it.

Done.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:51: }
On 2015/03/18 13:28:25, asgerf wrote:
> I think we try to avoid `visitXXX` names for methods that don't actually
> override a visitor method.

Done.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:57: return
'$indentation(FunctionDefinition $name $thisName($parameters) return\n'
On 2015/03/18 13:28:25, asgerf wrote:
> Long line

Done.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:88: return
'$indentation(ConstructorDefinition $name$thisName($parameters) return'
On 2015/03/18 13:28:25, asgerf wrote:
> Long line

Done.

https://codereview.chromium.org/1011383003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:454: return '<<${node}>>';
On 2015/03/18 13:28:25, asgerf wrote:
> It's not clear if this is leftover debugging code or intentional. In any case,
> there shouldn't be outcommented code.

I have reverted this change, and added a TODO to make the printer produce
something for malformed terms to assist in the debugging of malformed terms.

[asgerf, 2015-03-19 11:31:13]

Still LGTM after some clean up.

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend.dart:170:
//print(cpsDefinition.accept(new SExpressionStringifier()));
Clean up

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart:403: :*/
makeExp(exp.object, PRIMARY, beginStmt: beginStmt);
Clean up.

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:125: if (thisParameter != null
&& reference.definition == thisParameter) {
Seems unnecessary. Reference.definition should never be null.

https://codereview.chromium.org/1011383003/diff/20001/tests/language/paramete...
File tests/language/parameter_initializer_test.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/tests/language/paramete...
tests/language/parameter_initializer_test.dart:31: */
Clean up.

[asgerf, 2015-03-19 13:39:41]

RecursiveVisitor.visitFunctionDefinition and visitConstructorDefition should
visit the thisParameter.

ParentVisitor in ShrinkingReductions should set the parent pointer on the
thisParameter.

[sra1, 2015-03-19 16:54:25]

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend.dart:170:
//print(cpsDefinition.accept(new SExpressionStringifier()));
On 2015/03/19 11:31:13, asgerf wrote:
> Clean up

Done.

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart:403: :*/
makeExp(exp.object, PRIMARY, beginStmt: beginStmt);
On 2015/03/19 11:31:13, asgerf wrote:
> Clean up.

Done.

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:125: if (thisParameter != null
&& reference.definition == thisParameter) {
On 2015/03/19 11:31:13, asgerf wrote:
> Seems unnecessary. Reference.definition should never be null.

I'm going to keep as-is - you should not try to match 'This' if there is no
'this'.

https://codereview.chromium.org/1011383003/diff/20001/tests/language/paramete...
File tests/language/parameter_initializer_test.dart (right):

https://codereview.chromium.org/1011383003/diff/20001/tests/language/paramete...
tests/language/parameter_initializer_test.dart:31: */
On 2015/03/19 11:31:13, asgerf wrote:
> Clean up.

Done.

[sra1, 2015-03-19 17:10:15]

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

[karlklose, 2015-03-20 09:43:07]

karlklose@google.com changed reviewers:
+ karlklose@google.com

[karlklose, 2015-03-20 09:43:08]

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1279: //assert(target ==
irBuilder.state.thisParameter);
Remove commented out code.

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend.dart:170:
//print(cpsDefinition.accept(new SExpressionStringifier()));
Remove.

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:234: /*  List<Variable>
translatePhiArguments(List<cps_ir.Reference> args) {
Remove old code and comment.

[sra1, 2015-03-20 10:01:42]

Done (in redo CL: https://chromiumcodereview.appspot.com/1021813002/)

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1279: //assert(target ==
irBuilder.state.thisParameter);
On 2015/03/20 09:43:07, karlklose wrote:
> Remove commented out code.

Done.

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend.dart:170:
//print(cpsDefinition.accept(new SExpressionStringifier()));
On 2015/03/20 09:43:07, karlklose wrote:
> Remove.

Done.

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1011383003/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:234: /*  List<Variable>
translatePhiArguments(List<cps_ir.Reference> args) {
On 2015/03/20 09:43:07, karlklose wrote:
> Remove old code and comment.

Done.