Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10942029:
Resolve op when op= syntax is used. (Closed)
|
Created:
8 years, 3 months ago by Anton Muhin Modified:
8 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionResolve op when op= syntax is used.
Committed: https://code.google.com/p/dart/source/detail?r=12839
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); This will register '+=' as a binary operator. But '+=' is not a binary operator.
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); That's somewhat surprising, but it actually works as Selector.binaryOperator invokes Elements.constructOperatorName which in it's turn translates op= strings to op. On 2012/09/24 09:19:31, ahe wrote: > This will register '+=' as a binary operator. But '+=' is not a binary operator.
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); I agree this is surprising, and I don't think this should be happening. I would have cleaned this up a long time ago if I hadn't been behind on language features. On 2012/09/24 09:22:30, Anton Muhin wrote: > That's somewhat surprising, but it actually works as Selector.binaryOperator > invokes Elements.constructOperatorName which in it's turn translates op= strings > to op. > > On 2012/09/24 09:19:31, ahe wrote: > > This will register '+=' as a binary operator. But '+=' is not a binary > operator. >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); Peter, you should know this stuff better. Anyway, may I proceed with my CL? I'd rather fix the problem we have with dart2dart. Any other solution seems even worse than this one. On 2012/09/24 11:14:55, ahe wrote: > I agree this is surprising, and I don't think this should be happening. I would > have cleaned this up a long time ago if I hadn't been behind on language > features. > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > That's somewhat surprising, but it actually works as Selector.binaryOperator > > invokes Elements.constructOperatorName which in it's turn translates op= > strings > > to op. > > > > On 2012/09/24 09:19:31, ahe wrote: > > > This will register '+=' as a binary operator. But '+=' is not a binary > > operator. > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); On 2012/09/24 13:13:07, Anton Muhin wrote: > Peter, you should know this stuff better. > > Anyway, may I proceed with my CL? I'd rather fix the problem we have with > dart2dart. Any other solution seems even worse than this one. dart2dart is not a short-term priority, so we can take the time to do it right. > > On 2012/09/24 11:14:55, ahe wrote: > > I agree this is surprising, and I don't think this should be happening. I > would > > have cleaned this up a long time ago if I hadn't been behind on language > > features. > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > That's somewhat surprising, but it actually works as Selector.binaryOperator > > > invokes Elements.constructOperatorName which in it's turn translates op= > > strings > > > to op. > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > This will register '+=' as a binary operator. But '+=' is not a binary > > > operator. > > > > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); What would be your advice then On 2012/09/24 13:22:24, ahe wrote: > On 2012/09/24 13:13:07, Anton Muhin wrote: > > Peter, you should know this stuff better. > > > > Anyway, may I proceed with my CL? I'd rather fix the problem we have with > > dart2dart. Any other solution seems even worse than this one. > > dart2dart is not a short-term priority, so we can take the time to do it right. > > > > > On 2012/09/24 11:14:55, ahe wrote: > > > I agree this is surprising, and I don't think this should be happening. I > > would > > > have cleaned this up a long time ago if I hadn't been behind on language > > > features. > > > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > > That's somewhat surprising, but it actually works as > Selector.binaryOperator > > > > invokes Elements.constructOperatorName which in it's turn translates op= > > > strings > > > > to op. > > > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > > This will register '+=' as a binary operator. But '+=' is not a binary > > > > operator. > > > > > > > > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); Create a method that maps a compound operator to its corresponding user-definable operator. I already have a CL that does that: http://codereview.chromium.org/10905211/ Right now, landing that CL is not a priority for me. Updating co19 and implementing M1 language features is. So if you cannot wait for me to land the CL, I suggest you hack around this in dart2dart by registering all operators up front. On 2012/09/24 13:23:31, Anton Muhin wrote: > What would be your advice then > > On 2012/09/24 13:22:24, ahe wrote: > > On 2012/09/24 13:13:07, Anton Muhin wrote: > > > Peter, you should know this stuff better. > > > > > > Anyway, may I proceed with my CL? I'd rather fix the problem we have with > > > dart2dart. Any other solution seems even worse than this one. > > > > dart2dart is not a short-term priority, so we can take the time to do it > right. > > > > > > > > On 2012/09/24 11:14:55, ahe wrote: > > > > I agree this is surprising, and I don't think this should be happening. I > > > would > > > > have cleaned this up a long time ago if I hadn't been behind on language > > > > features. > > > > > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > > > That's somewhat surprising, but it actually works as > > Selector.binaryOperator > > > > > invokes Elements.constructOperatorName which in it's turn translates op= > > > > strings > > > > > to op. > > > > > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > > > This will register '+=' as a binary operator. But '+=' is not a binary > > > > > operator. > > > > > > > > > > > > > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); Or even better: take Elements.mapToUserOperator from CL 10905211 and use it here. You should notice that the original source string already exists, see line 1503. So try something like this on line 1503: SourceString operatorName = node.assignmentOperator.source; String source = operatorName.stringValue; And then here use this: registerBinaryOperator(Elements.mapToUserOperator(operatorName)); On 2012/09/24 13:50:30, ahe wrote: > Create a method that maps a compound operator to its corresponding > user-definable operator. > > I already have a CL that does that: http://codereview.chromium.org/10905211/ > > Right now, landing that CL is not a priority for me. Updating co19 and > implementing M1 language features is. So if you cannot wait for me to land the > CL, I suggest you hack around this in dart2dart by registering all operators up > front. > > > On 2012/09/24 13:23:31, Anton Muhin wrote: > > What would be your advice then > > > > On 2012/09/24 13:22:24, ahe wrote: > > > On 2012/09/24 13:13:07, Anton Muhin wrote: > > > > Peter, you should know this stuff better. > > > > > > > > Anyway, may I proceed with my CL? I'd rather fix the problem we have with > > > > dart2dart. Any other solution seems even worse than this one. > > > > > > dart2dart is not a short-term priority, so we can take the time to do it > > right. > > > > > > > > > > > On 2012/09/24 11:14:55, ahe wrote: > > > > > I agree this is surprising, and I don't think this should be happening. > I > > > > would > > > > > have cleaned this up a long time ago if I hadn't been behind on language > > > > > features. > > > > > > > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > > > > That's somewhat surprising, but it actually works as > > > Selector.binaryOperator > > > > > > invokes Elements.constructOperatorName which in it's turn translates > op= > > > > > strings > > > > > > to op. > > > > > > > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > > > > This will register '+=' as a binary operator. But '+=' is not a > binary > > > > > > operator. > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); Thanks a lot, Peter, let me give it a try. On 2012/09/24 16:07:55, ahe wrote: > Or even better: take Elements.mapToUserOperator from CL 10905211 and use it > here. > > You should notice that the original source string already exists, see line 1503. > > So try something like this on line 1503: > > SourceString operatorName = node.assignmentOperator.source; > String source = operatorName.stringValue; > > And then here use this: > > registerBinaryOperator(Elements.mapToUserOperator(operatorName)); > > On 2012/09/24 13:50:30, ahe wrote: > > Create a method that maps a compound operator to its corresponding > > user-definable operator. > > > > I already have a CL that does that: http://codereview.chromium.org/10905211/ > > > > Right now, landing that CL is not a priority for me. Updating co19 and > > implementing M1 language features is. So if you cannot wait for me to land the > > CL, I suggest you hack around this in dart2dart by registering all operators > up > > front. > > > > > > On 2012/09/24 13:23:31, Anton Muhin wrote: > > > What would be your advice then > > > > > > On 2012/09/24 13:22:24, ahe wrote: > > > > On 2012/09/24 13:13:07, Anton Muhin wrote: > > > > > Peter, you should know this stuff better. > > > > > > > > > > Anyway, may I proceed with my CL? I'd rather fix the problem we have > with > > > > > dart2dart. Any other solution seems even worse than this one. > > > > > > > > dart2dart is not a short-term priority, so we can take the time to do it > > > right. > > > > > > > > > > > > > > On 2012/09/24 11:14:55, ahe wrote: > > > > > > I agree this is surprising, and I don't think this should be > happening. > > I > > > > > would > > > > > > have cleaned this up a long time ago if I hadn't been behind on > language > > > > > > features. > > > > > > > > > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > > > > > That's somewhat surprising, but it actually works as > > > > Selector.binaryOperator > > > > > > > invokes Elements.constructOperatorName which in it's turn translates > > op= > > > > > > strings > > > > > > > to op. > > > > > > > > > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > > > > > This will register '+=' as a binary operator. But '+=' is not a > > binary > > > > > > > operator. > > > > > > > > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10942029/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1560: registerBinaryOperator(new SourceString(source)); Done, ptal On 2012/09/24 16:09:18, Anton Muhin wrote: > Thanks a lot, Peter, let me give it a try. > > On 2012/09/24 16:07:55, ahe wrote: > > Or even better: take Elements.mapToUserOperator from CL 10905211 and use it > > here. > > > > You should notice that the original source string already exists, see line > 1503. > > > > So try something like this on line 1503: > > > > SourceString operatorName = node.assignmentOperator.source; > > String source = operatorName.stringValue; > > > > And then here use this: > > > > registerBinaryOperator(Elements.mapToUserOperator(operatorName)); > > > > On 2012/09/24 13:50:30, ahe wrote: > > > Create a method that maps a compound operator to its corresponding > > > user-definable operator. > > > > > > I already have a CL that does that: http://codereview.chromium.org/10905211/ > > > > > > Right now, landing that CL is not a priority for me. Updating co19 and > > > implementing M1 language features is. So if you cannot wait for me to land > the > > > CL, I suggest you hack around this in dart2dart by registering all operators > > up > > > front. > > > > > > > > > On 2012/09/24 13:23:31, Anton Muhin wrote: > > > > What would be your advice then > > > > > > > > On 2012/09/24 13:22:24, ahe wrote: > > > > > On 2012/09/24 13:13:07, Anton Muhin wrote: > > > > > > Peter, you should know this stuff better. > > > > > > > > > > > > Anyway, may I proceed with my CL? I'd rather fix the problem we have > > with > > > > > > dart2dart. Any other solution seems even worse than this one. > > > > > > > > > > dart2dart is not a short-term priority, so we can take the time to do it > > > > right. > > > > > > > > > > > > > > > > > On 2012/09/24 11:14:55, ahe wrote: > > > > > > > I agree this is surprising, and I don't think this should be > > happening. > > > I > > > > > > would > > > > > > > have cleaned this up a long time ago if I hadn't been behind on > > language > > > > > > > features. > > > > > > > > > > > > > > On 2012/09/24 09:22:30, Anton Muhin wrote: > > > > > > > > That's somewhat surprising, but it actually works as > > > > > Selector.binaryOperator > > > > > > > > invokes Elements.constructOperatorName which in it's turn > translates > > > op= > > > > > > > strings > > > > > > > > to op. > > > > > > > > > > > > > > > > On 2012/09/24 09:19:31, ahe wrote: > > > > > > > > > This will register '+=' as a binary operator. But '+=' is not a > > > binary > > > > > > > > operator. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
