Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(448)

Issue 1020243002: cps_ir: interceptor calling convention, operator== prologue (Closed)

Created:
5 years, 9 months ago by sra1
Modified:
5 years, 6 months ago
CC:
reviews_dartlang.org
Target Ref:
refs/remotes/git-svn
Visibility:
Public.

Description

Methods using interceptor calling convention take an explicit interceptor argument. operator== checks argument against null Closed this issue - the work has been committed as part of another CL

Patch Set 1 : #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -24 lines) Patch
M pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart View 3 chunks +7 lines, -1 line 1 comment Download
M pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart View 1 chunk +2 lines, -0 lines 0 comments Download
M pkg/compiler/lib/src/js_backend/codegen/glue.dart View 1 chunk +4 lines, -0 lines 1 comment Download
M pkg/compiler/lib/src/js_backend/codegen/task.dart View 1 chunk +0 lines, -7 lines 0 comments Download
M pkg/compiler/lib/src/js_backend/codegen/unsugar.dart View 1 2 6 chunks +138 lines, -16 lines 9 comments Download

Messages

Total messages: 8 (2 generated)
sra1
The generated code for operator== looks like: $eq: function(receiver, other) { var v0, v1, ...; ...
5 years, 9 months ago (2015-03-23 21:24:41 UTC) #3
asgerf
On 2015/03/23 21:24:41, sra1 wrote: > The generated code for operator== looks like: > > ...
5 years, 9 months ago (2015-03-24 09:03:41 UTC) #4
Kevin Millikin (Google)
On 2015/03/24 09:03:41, asgerf wrote: > In the tree language, we are going to eliminate ...
5 years, 9 months ago (2015-03-24 10:27:47 UTC) #5
sra1
On 2015/03/24 10:27:47, kmillikin wrote: > On 2015/03/24 09:03:41, asgerf wrote: > > > In ...
5 years, 9 months ago (2015-03-26 01:16:57 UTC) #6
sra1
PTAL.
5 years, 9 months ago (2015-03-26 01:17:38 UTC) #7
Kevin Millikin (Google)
5 years, 9 months ago (2015-03-26 15:14:17 UTC) #8
LGTM with a bunch of suggestions and a few things to fix.

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

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2475: Selector selector =
new Selector.fromElement(element);
Inadvertent change?  selector is only used in the commented-out code below.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:158: bool
requiresEqNullCheck(FunctionElement element) {
Can we avoid having two different names (requiresEqNullCheck vs.
!operatorEqHandlesNullArgument) for the same thing in two different places?

If one is obviously better, we should just adopt it in both places.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/unsugar.dart (right):

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:19: final String name;
Consider hardcoding the name 'receiver' here rather than at construction time.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:34: ///  - Use current or
new interceptor at call sites taht use interceptor calling
'taht' ==> 'that'

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:36: ///  - Rewrite
operator== to test the argument for null.
'first argument' or 'receiver argument'?

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:76:
explicitReceiverParameter.firstRef = thisParameter.firstRef;
substituteFor will do this (and doing it here is only correct in the case when
thisParameter.firstRef == null because we know there are no references to
explicitRecevierParameter).

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:77: thisParameter.firstRef
= null;
And we should almost certainly do this in substituteFor as well.

I think this is the first time we're substituting x for y without removing y
from the CPS term, so we just haven't thought about the issue of what is seen as
uses of y after the substitution.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:89: Constant makeTrue() {
Oooh, these seem like good utilities to have in class ir.Constant.  What do you
think?

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:154: // TODO(karlklose):
should we rewrite all selectors?  sra: No.  It would add
I don't understand the question or the answer.  Maybe we three should agree on
what the question is, what the answer is, and get rid of the comment.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:167: Reference<Primitive>
receiverRef = node.receiver;
This is unused.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:175: Set<ClassElement>
interceptedClasses =
I think there's enough shared code here and above that it could profitably be
something like:

Primitive newReceiver;
if (receiver == explicitReceiverParameter) {
  // If the receiver is the explicit receiver, we are calling a method in the
  // same interceptor.
  //   receiver.foo(...)  -->  this.foo(receiver, ...);
  newReceiver = thisParameter;
} else {
  // TODO(sra): Move the computation of interceptedClasses to a much later
  // phase and take into account the remaining uses of the interceptor.
  Set<ClassElement> interceptedClasses =
      _glue.getInterceptedClassesOn(selector);
  _glue.registerSpecializedGetInterceptor(interceptedClasses);
  newReceiver = new Interceptor(receiver, interceptedClasses);
  insertLetPrim(newReceiver, node);
}
node.arguments.insert(0, node.receiver);
node.callingConvention = CallingConvention.JS_INTERCEPTED;
node.receiver = new Reference<Primitive>(newReceiver);
assert(node.isValid);

Powered by Google App Engine
This is Rietveld 408576698