Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27953002:
Refine spec for TypedefMirror.value and for ClosureMirror.findInContext. (Closed)
|
Created:
7 years, 2 months ago by gbracha Modified:
7 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionRfine spec for TypedefMirror.value and for ClosureMirror.findInContext.
R=ahe@google.com, rmacnak@google.com
Committed: https://code.google.com/p/dart/source/detail?r=30386
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 12 (0 generated)
This shows what I would like to see as the spec for findInContext. I am NOT proposing to push this out as is. We should decide what we want and edit this as needed.
LGTM https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.d... sdk/lib/mirrors/mirrors.dart:639: * If the reflectee is not an actual closure, returns null. How can that happen? https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.d... sdk/lib/mirrors/mirrors.dart:645: * an ArgumentError is thrown. Long term, I like this. What about operators? Short-term, I would like something like this: If *s* is not a an identifier (or an operator), an ArgumentError is thrown. If *s* is a library prefix, an ArgumentError is thrown.
Ok, I think this is now in a form where I could push it out. Agreed? https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.d... sdk/lib/mirrors/mirrors.dart:639: * If the reflectee is not an actual closure, returns null. On 2013/10/22 12:47:18, ahe wrote: > How can that happen? We generate a closure mirror for anything that either is a closure, a function or emulates a function (i.e, has a call() method. Ideally, findInContext would work for the last category, but that is future work I think. So for now, it just answers null. https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.d... sdk/lib/mirrors/mirrors.dart:645: * an ArgumentError is thrown. On 2013/10/22 12:47:18, ahe wrote: > Long term, I like this. What about operators? Operators do not have a meaning as an expression. That said, it turns out this actually works (Michael was more consistent than the language is) and will closurize the operator just like an ordinary instance method. I suppose I should document that. > > Short-term, I would like something like this: > > If *s* is not a an identifier (or an operator), an ArgumentError is thrown. If > *s* is a library prefix, an ArgumentError is thrown. I figured.
https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/27953002/diff/31001/sdk/lib/mirrors/mirrors.d... sdk/lib/mirrors/mirrors.dart:639: * If the reflectee is not an actual closure, returns null. On 2013/10/22 23:40:29, gbracha wrote: > On 2013/10/22 12:47:18, ahe wrote: > > How can that happen? > > We generate a closure mirror for anything that either is a closure, a function > or emulates a function (i.e, has a call() method. Ideally, findInContext would > work for the last category, but that is future work I think. So for now, it just > answers null. See also https://codereview.chromium.org/26777002/
https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... sdk/lib/mirrors/mirrors.dart:652: * If the reflectee is not an actual closure, returns null. I think "actual closure" could be made a little more precise somehow. I'm not sure if we the terminology already. As far as I can tell, the language specification uses the terminology "function" to describe what you call "actual closure". I'm sure implicit/tear-off closures complicate this picture. So how about: If the reflectee is not an actual function, that is, a declared class which implements [Function], this method returns null. https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... sdk/lib/mirrors/mirrors.dart:658: * an ArgumentError is thrown. Short-term, also throw an argument error if s is a prefix. https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... sdk/lib/mirrors/mirrors.dart:667: * If *s* is the name of an operator, let *v* be (x) => this s x. I don't understand this part.
On 2013/10/23 17:58:59, ahe wrote: > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > File sdk/lib/mirrors/mirrors.dart (right): > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > sdk/lib/mirrors/mirrors.dart:652: * If the reflectee is not an actual closure, > returns null. > I think "actual closure" could be made a little more precise somehow. I'm not > sure if we the terminology already. > > As far as I can tell, the language specification uses the terminology "function" > to describe what you call "actual closure". I'm sure implicit/tear-off closures > complicate this picture. I can call it function. Since tear-offs are defined via function literals, I think that is ok. > > So how about: > > If the reflectee is not an actual function, that is, a declared class which > implements [Function], this method returns null. Don't follow. You mean If the reflectee is not an actual function, that is, the reflectee is a user defined class which implements [Function], this method returns null. > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > sdk/lib/mirrors/mirrors.dart:658: * an ArgumentError is thrown. > Short-term, also throw an argument error if s is a prefix. Ok. > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > sdk/lib/mirrors/mirrors.dart:667: * If *s* is the name of an operator, let *v* > be (x) => this s x. > I don't understand this part. Suppose s= + v = (x) => this + x in other words, it is a closurized operator. I need this because + is not an expression.
On 2013/10/23 19:10:04, gbracha wrote: > On 2013/10/23 17:58:59, ahe wrote: > > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > > File sdk/lib/mirrors/mirrors.dart (right): > > > > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > > sdk/lib/mirrors/mirrors.dart:652: * If the reflectee is not an actual closure, > > returns null. > > I think "actual closure" could be made a little more precise somehow. I'm not > > sure if we the terminology already. > > > > As far as I can tell, the language specification uses the terminology > "function" > > to describe what you call "actual closure". I'm sure implicit/tear-off > closures > > complicate this picture. > > I can call it function. Since tear-offs are defined via function literals, I > think that is ok. > > > > So how about: > > > > If the reflectee is not an actual function, that is, a declared class which > > implements [Function], this method returns null. > > Don't follow. You mean > > If the reflectee is not an actual function, that is, the reflectee is a user > defined class which > implements [Function], this method returns null. > > > > > > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > > sdk/lib/mirrors/mirrors.dart:658: * an ArgumentError is thrown. > > Short-term, also throw an argument error if s is a prefix. > > Ok. > > > > > > https://codereview.chromium.org/27953002/diff/121001/sdk/lib/mirrors/mirrors.... > > sdk/lib/mirrors/mirrors.dart:667: * If *s* is the name of an operator, let *v* > > be (x) => this s x. > > I don't understand this part. > > Suppose s= + > > v = (x) => this + x > > in other words, it is a closurized operator. I need this because + is not an > expression. PTAL.
LGTM
https://codereview.chromium.org/27953002/diff/201001/sdk/lib/mirrors/mirrors.... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/27953002/diff/201001/sdk/lib/mirrors/mirrors.... sdk/lib/mirrors/mirrors.dart:671: * If *s* is the name of an operator, let *v* be (x) => this s x. OK, now I understand this better, I'm concerned that findInContext is for looking up values, not declarations. I had overlooked that previously. I don't see dart2js being able to support this method in release 1.0. I think we need to remove this method from the API for now.
Ok, lobotomized findInContext. PTAL and confirm.
LGTM!
Message was sent while issue was closed.
Committed patchset #7 manually as r30386 (presubmit successful). |
