Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1122983002:
Improve documentation for MirrorsUsed. (Closed)
|
Created:
5 years, 7 months ago by herhut Modified:
5 years, 7 months ago CC:
reviews_dartlang.org, ahe Target Ref:
refs/remotes/git-svn Visibility:
Public. |
DescriptionImprove documentation for MirrorsUsed.
BUG= http://dartbug.com/15656
R=floitsch@google.com, kathyw@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=45585
Patch Set 1 #
Total comments: 65
Patch Set 2 : Comments and fixes. #
Total comments: 10
Patch Set 3 : More language fixes. #Messages
Total messages: 13 (2 generated)
herhut@google.com changed reviewers: + floitsch@google.com, kathyw@google.com
Here is a first go at describing what the implementation of MirrorsUsed in dart2js actually does.
LGTM!
Some suggestions for wording, but I'm very happy to see these docs! https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1214: * see the comments for the respective fields [symbols]. [targets] and delete "the respective fields " . -> , https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1215: * [metaTargets]. what about [override]? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1219: * example example -> example: https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1245: * Dart2js currently supports the below formats to specify symbols: below -> following https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1247: * * A [List] of [String] constants representing symbol names, e.g., A -> A constant ? (it must be a const List, right?) https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1249: * * A single [String] constant whose value is a comma separated list of comma separated -> comma-separated https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1252: * Specifying this option turns off the following warnings emitted by this option -> the `symbols` field https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1258: * For example, if using [noSuchMethod] to interact with a database, extract using -> you're using https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1260: * if using [noSuchMethod] to interact with another language (JavaScript, for using -> you're using https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1261: * example) extract all the identifiers from API used and include them in API -> the API you use https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1279: * Dart2js currently supports the below formats to specify targets: Same comments as above: below -> following [List] -> constant [List] (?) comma separated -> comma-separated https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1300: * A brief example would be nice here. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1374: * For example -> For example, the following code marks all targets in... library. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1380: * However, Delete previous two lines (moved, actually). Change this one to: However, the following code would require.... https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1384: * would require the use of the `Reflectable` class from the current Delete these 2 lines (move up above, actually).
lrn@google.com changed reviewers: + lrn@google.com
https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1249: * * A single [String] constant whose value is a comma separated list of comma + whitespace separated apparently? Are multiple whitespaces allowed? Are whitespaces allowed before the comma? Before first value and/or after last? Are empty entries allowed ("x,,y")? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1275: * targets used by the library to which this metadata applies. What kinds of declarations can be targets. It's stated at the bottom, consider moving it up here. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1289: * 1. If the qualified name matches a library name, the matching library is "matches" meaning? I guess equals-after-canonicalization (removing whitespace). Is whitespaces allowed in qualified names? Around commas? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1292: * with a `.` and is a library name. ends with -> ends just before (Otherwise the prefix would end with '.' and can't be a valid library name). https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1296: * 4. Split the remaining suffix into a list of [String] using `.` as a Split the remaining suffix (the entire name in case item 3 above was used) into ... (non-mathematicians can have a problem with accepting suffices that are not proper :) https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1298: * 5. Select all targets in the current scope whose name matches a [String] "matches" means? Equals for non-setters or equals-without-the-= for setters? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1299: * from the list. Step 5 looks wrong. Not saying it's not what's happening, but it sure *looks* wrong! As I read it "foo.bar.baz.qux" can match a library named "foo.bar", and then match either "baz" or "qux" declared in that library - that is, something with fully qualified name "foo.bar.qux"? But it won't match the qux in "class baz { static var qux; }" in the library? (And methods can't be targeted directly). That is, the '.'s in the suffix are used as separators between equivalent names, not as a descending path. That's ... unconventional. So if this is really true (which I hear that it is), maybe add a comment saying "yes, this IS weird, it's not you misunderstanding it". https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1316: * The following text is non-normative: What does that mean? Where it the normative definition? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1322: * the library's classes a valid metadata annotation to enable reflection. a valid metadata annotation -> valid metadata annotations https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1324: * If a class specified in [metaTargets] is used as metadata annotation If an instance of a class ... https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1325: * on a class, field or method, that class, field or method is added to But not library? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1359: * applies to the other libraries (all libraries if "*" is used). In So it *does* apply to itself if "*" is used? And it won't remove itself in that case? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1361: * it will be shadowed by the override. What if you specify yourself without using "*"? library foo; @MirrorsUsed(....) @MirrorsUsed(..., "override":"foo,bar"); import "dart:mirrors"; Will this remove the MirrorsUsed annotations from "foo"? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1361: * it will be shadowed by the override. The word "shadow" suggests an ordering where the original is still there, but the new one is used instead. What if two libraries both override a third - will the third library have zero, one or two annotations that apply? If one, which one? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1364: * an empty annotation will be assumed. That's not what "shadow each other" means (if it means anything). Drop the "shadow" part of the explanation. "an empty annotation" -> both will be considered to have no `MirrorsUsed` annotation. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1366: * The following text is non-normative: Where is the normative text then? https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1386: */ So, if I get this right: Override specifies a set of libraries, or "*" for all libraries. To apply the override: - First find all libraries that are affected by an override. - Then remove the MirrorsUsed annotations on dart:mirrors imports of those libraries (except for the declaring library when using "*"?). - Then, for all remaining MirrorsUsed annotations with an override: - Apply the override MirrorsUsed annotation to dart:mirror imports on the libraries they specify. - From here on, ignore "override". There is still some vagueness about whether an override can remove itself.
PTAL I had to rewrite the section on override after I realised it does not actually override. Kathy, language screening of that section would be highly welcome. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1214: * see the comments for the respective fields [symbols]. [targets] and On 2015/05/05 19:24:57, Kathy Walrath wrote: > delete "the respective fields " > > . -> , Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1215: * [metaTargets]. On 2015/05/05 19:24:57, Kathy Walrath wrote: > what about [override]? Good catch, thanks! https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1219: * example On 2015/05/05 19:24:57, Kathy Walrath wrote: > example -> example: Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1245: * Dart2js currently supports the below formats to specify symbols: On 2015/05/05 19:24:58, Kathy Walrath wrote: > below -> following Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1247: * * A [List] of [String] constants representing symbol names, e.g., On 2015/05/05 19:24:57, Kathy Walrath wrote: > A -> A constant > ? (it must be a const List, right?) Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1249: * * A single [String] constant whose value is a comma separated list of On 2015/05/05 19:24:58, Kathy Walrath wrote: > comma separated -> comma-separated Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1249: * * A single [String] constant whose value is a comma separated list of On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > comma + whitespace separated apparently? > Are multiple whitespaces allowed? Are whitespaces allowed before the comma? > Before first value and/or after last? Are empty entries allowed ("x,,y")? This is meant to be a documentation, not a formal specification. I don't think adding such detail is overly helpful, especially as the example shows that whitespace is OK. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1252: * Specifying this option turns off the following warnings emitted by On 2015/05/05 19:24:57, Kathy Walrath wrote: > this option -> the `symbols` field Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1258: * For example, if using [noSuchMethod] to interact with a database, extract On 2015/05/05 19:24:57, Kathy Walrath wrote: > using -> you're using Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1260: * if using [noSuchMethod] to interact with another language (JavaScript, for On 2015/05/05 19:24:58, Kathy Walrath wrote: > using -> you're using Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1261: * example) extract all the identifiers from API used and include them in On 2015/05/05 19:24:57, Kathy Walrath wrote: > API -> the API you use Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1275: * targets used by the library to which this metadata applies. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > What kinds of declarations can be targets. > > It's stated at the bottom, consider moving it up here. It is not stated here because it is not formally defined. I have inherited this format of the documentation and wanted to avoid adding normative statements about what MirrorsUsed covers. I have moved the paragraph up, though. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1279: * Dart2js currently supports the below formats to specify targets: On 2015/05/05 19:24:58, Kathy Walrath wrote: > Same comments as above: > > below -> following > [List] -> constant [List] (?) > comma separated -> comma-separated Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1289: * 1. If the qualified name matches a library name, the matching library is On 2015/05/06 07:41:20, Lasse Reichstein Nielsen wrote: > "matches" meaning? > I guess equals-after-canonicalization (removing whitespace). Is whitespaces > allowed in qualified names? Around commas? ...not a spec. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1292: * with a `.` and is a library name. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > ends with -> ends just before > (Otherwise the prefix would end with '.' and can't be a valid library name). Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1296: * 4. Split the remaining suffix into a list of [String] using `.` as a On 2015/05/06 07:41:20, Lasse Reichstein Nielsen wrote: > Split the remaining suffix (the entire name in case item 3 above was used) into > ... > > (non-mathematicians can have a problem with accepting suffices that are not > proper :) Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1298: * 5. Select all targets in the current scope whose name matches a [String] On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > "matches" means? Equals for non-setters or equals-without-the-= for setters? without the = for setters. Again this is not a spec. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1299: * from the list. On 2015/05/06 07:41:20, Lasse Reichstein Nielsen wrote: > Step 5 looks wrong. Not saying it's not what's happening, but it sure *looks* > wrong! > > As I read it "foo.bar.baz.qux" can match a library named "foo.bar", and then > match either "baz" or "qux" declared in that library - that is, something with > fully qualified name "foo.bar.qux"? > But it won't match the qux in "class baz { static var qux; }" in the library? > (And methods can't be targeted directly). > > That is, the '.'s in the suffix are used as separators between equivalent names, > not as a descending path. That's ... unconventional. > > So if this is really true (which I hear that it is), maybe add a comment saying > "yes, this IS weird, it's not you misunderstanding it". This is how it is and I added an example to highlight this oddity. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1300: * On 2015/05/05 19:24:58, Kathy Walrath wrote: > A brief example would be nice here. Added. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1316: * The following text is non-normative: On 2015/05/06 07:41:20, Lasse Reichstein Nielsen wrote: > What does that mean? Where it the normative definition? There is none. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1322: * the library's classes a valid metadata annotation to enable reflection. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > a valid metadata annotation -> valid metadata annotations Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1324: * If a class specified in [metaTargets] is used as metadata annotation On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > If an instance of a class ... Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1325: * on a class, field or method, that class, field or method is added to On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > But not library? Good catch, thanks! https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1359: * applies to the other libraries (all libraries if "*" is used). In On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > So it *does* apply to itself if "*" is used? > And it won't remove itself in that case? I had to rewrite the entire section after looking at the code again. It is actually just adding mirrorsUsed annotations. There is no overriding in any form. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1361: * it will be shadowed by the override. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > The word "shadow" suggests an ordering where the original is still there, but > the new one is used instead. > > What if two libraries both override a third - will the third library have zero, > one or two annotations that apply? If one, which one? Removed. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1361: * it will be shadowed by the override. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > The word "shadow" suggests an ordering where the original is still there, but > the new one is used instead. > > What if two libraries both override a third - will the third library have zero, > one or two annotations that apply? If one, which one? Removed. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1364: * an empty annotation will be assumed. On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > That's not what "shadow each other" means (if it means anything). > Drop the "shadow" part of the explanation. > "an empty annotation" -> both will be considered to have no `MirrorsUsed` > annotation. Removed. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1366: * The following text is non-normative: On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > Where is the normative text then? Does not exist. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1374: * For example On 2015/05/05 19:24:57, Kathy Walrath wrote: > -> For example, the following code marks all targets in... library. Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1380: * However, On 2015/05/05 19:24:58, Kathy Walrath wrote: > Delete previous two lines (moved, actually). Change this one to: > > However, the following code would require.... Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1384: * would require the use of the `Reflectable` class from the current On 2015/05/05 19:24:58, Kathy Walrath wrote: > Delete these 2 lines (move up above, actually). Done. https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1386: */ On 2015/05/06 07:41:21, Lasse Reichstein Nielsen wrote: > So, if I get this right: > Override specifies a set of libraries, or "*" for all libraries. > To apply the override: > - First find all libraries that are affected by an override. > - Then remove the MirrorsUsed annotations on dart:mirrors imports of those > libraries (except for the declaring library when using "*"?). > - Then, for all remaining MirrorsUsed annotations with an override: > - Apply the override MirrorsUsed annotation to dart:mirror imports on the > libraries they specify. > - From here on, ignore "override". > > There is still some vagueness about whether an override can remove itself. Removed.
https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. new #{name} -> new Symbol("name") ? I don't think "new #x" is a thing. https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1401: * without annotation. ... because otherwise it would work exactly the same way without the override parameter. Much simpler :)
lgtm https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:1300: * On 2015/05/06 14:24:16, herhut wrote: > On 2015/05/05 19:24:58, Kathy Walrath wrote: > > A brief example would be nice here. > > Added. That helps. Thanks! https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. On 2015/05/06 20:14:20, Lasse Reichstein Nielsen wrote: > new #{name} -> new Symbol("name") ? > I don't think "new #x" is a thing. Is `#name` the same as `new Symbol("name")`? If so, mention both. https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1344: * metadata annotation on a library, class, field or method, that libary, libary -> library
https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. #name is the same as 'const Symbol ("name")'. I think it's only using 'new Symbol' that causes large output.
So I am happy with this now. Lasse? https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. On 2015/05/06 20:14:20, Lasse Reichstein Nielsen wrote: > new #{name} -> new Symbol("name") ? > I don't think "new #x" is a thing. Well, this is just a copy of the strings from warnings. The #{name} is the placeholder that will be replaced by 'Symbol'. I will fix it. https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. On 2015/05/06 20:54:25, Kathy Walrath wrote: > On 2015/05/06 20:14:20, Lasse Reichstein Nielsen wrote: > > new #{name} -> new Symbol("name") ? > > I don't think "new #x" is a thing. > > Is `#name` the same as `new Symbol("name")`? If so, mention both. See above. https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1255: * * Using "new #{name}" may result in larger output. On 2015/05/07 06:38:14, Lasse Reichstein Nielsen wrote: > #name is the same as 'const Symbol ("name")'. I think it's only using 'new > Symbol' that causes large output. Correct. https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1344: * metadata annotation on a library, class, field or method, that libary, On 2015/05/06 20:54:25, Kathy Walrath wrote: > libary -> library Thanks! https://codereview.chromium.org/1122983002/diff/20001/sdk/lib/mirrors/mirrors... sdk/lib/mirrors/mirrors.dart:1401: * without annotation. On 2015/05/06 20:14:20, Lasse Reichstein Nielsen wrote: > ... because otherwise it would work exactly the same way without the override > parameter. > > Much simpler :) Done.
Still unhappy with everything being non-normative, but that's not a blocker :) LGTM
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 45585 (presubmit successful). |
