Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11864010:
Improve checking of patches. (Closed)
|
Created:
7 years, 11 months ago by Johnni Winther Modified:
7 years, 10 months ago CC:
reviews_dartlang.org, kasperl Visibility:
Public. |
DescriptionImprove checking of patches.
Committed: https://code.google.com/p/dart/source/detail?r=18137
Patch Set 1 #
Total comments: 39
Patch Set 2 : Rebased #
Total comments: 1
Patch Set 3 : Rebased #
Total comments: 6
Patch Set 4 : Rebased #Patch Set 5 : 'spannable' => 'node' #Patch Set 6 : Rebased #Patch Set 7 : Rebased (again) #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (right): https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable spannable, [Uri uri]); Slip from https://codereview.chromium.org/11854009/ https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/lib/isolate_patch.dart (right): https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/lib/isolate_patch.dart:6: This was what caused the need for the updated checking.
LGTM, but I'd also wait for Peter's review. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:196: if (!_isPatchElement(patch)) continue; How can that be? https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:402: } indentation https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:461: origin.patch = patch; Use setPatch to hide implementation details? https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:581: patch.origin = origin; Add a setOrigin? https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... File tests/compiler/dart2js/patch_test.dart (right): https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:48: bool isRegular: false}) { What's 'isRegular'? Maybe add a comment.
Private methods are bad. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:196: if (!_isPatchElement(patch)) continue; This would be an injected member, right? I assume it would be a private member and that is checked elsewhere. But perhaps you could assert that? https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:393: void _patchElement(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:403: if (!(patch.isClass() || Let me suggest a shorter alternative version of this method. First we check that the patch has certain properties, then we check that the origin has almost the same properties. And then we switch on these same properties to report errors once again. I would have written the code like this: if (patch.isClass()) { tryPatchClass(listener, origin, patch); } else if (patch.isGetter()) { tryPatchGetter(listener, origin, patch); } else if (patch.isSetter()) { tryPatchSetter(listener, origin, patch); } else if (patch.isConstructor()) { tryPatchConstructor(listener, origin, patch); } else if(patch.isFunction()) { tryPatchFunction(listener, origin, patch); } else { listener.reportMessage( listener.spanFromSpannable(patch), leg.MessageKind.PATCH_NONPATCHABLE.error(), api.Diagnostic.ERROR); } This code is equivalent, but rather than three "switches", you have only one (so this is slightly faster). In addition, you don't need any asserts to check what you just checked (assert on line 432 is rechecking lines 403-406). This makes the code easier to extend to handle a new kind of element. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:437: void _tryPatchClass(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:454: void _patchClass(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:465: void _tryPatchGetter(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:468: if (!origin.isAbstractField()) { I'd share this check between tryPatchGetter and tryPatchSetter. Normally, I wouldn't, but in this case, the audience is comprised exclusively of Dart library maintainers, so we don't need fine granularity of messages. This made me think of something related: perhaps we should make sure that all these messages are all prefixed with PATCH_. This allows us to group these error codes together and let users of the API know that these errors are internal and additional documentation shouldn't be necessary. Also, translation should be a low priority. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:494: void _tryPatchSetter(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:523: void _tryPatchConstructor(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:540: void _tryPatchFunction(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:557: void _patchFunction(leg.DiagnosticListener listener, I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:584: bool _isPatchElement(Element element) { I'd like to see a unit test of this method. https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... File tests/compiler/dart2js/patch_test.dart (right): https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:395: """ Why a multiline string? https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:405: compiler.errors[0].message.kind == MessageKind.PATCH_NON_EXISTING); I don't think you're testing the positions. This applies to all the tests below. Let's try to think of a way we can make it easy to test positions. https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:445: compiler.errors[0].message.kind == MessageKind.PATCH_NONPATCHABLE); This, and many tests below, do not test that you get the informational message that is generated.
On 2013/01/21 10:53:35, ahe wrote: > Private methods are bad. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right): > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:196: if > (!_isPatchElement(patch)) continue; > This would be an injected member, right? I assume it would be a private member > and that is checked elsewhere. But perhaps you could assert that? > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:393: void > _patchElement(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:403: if > (!(patch.isClass() || > Let me suggest a shorter alternative version of this method. > > First we check that the patch has certain properties, then we check that the > origin has almost the same properties. And then we switch on these same > properties to report errors once again. > > I would have written the code like this: > > if (patch.isClass()) { > tryPatchClass(listener, origin, patch); > } else if (patch.isGetter()) { > tryPatchGetter(listener, origin, patch); > } else if (patch.isSetter()) { > tryPatchSetter(listener, origin, patch); > } else if (patch.isConstructor()) { > tryPatchConstructor(listener, origin, patch); > } else if(patch.isFunction()) { > tryPatchFunction(listener, origin, patch); > } else { > listener.reportMessage( > listener.spanFromSpannable(patch), > leg.MessageKind.PATCH_NONPATCHABLE.error(), > api.Diagnostic.ERROR); > } > > This code is equivalent, but rather than three "switches", you have only one (so > this is slightly faster). In addition, you don't need any asserts to check what > you just checked (assert on line 432 is rechecking lines 403-406). This makes > the code easier to extend to handle a new kind of element. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:437: void > _tryPatchClass(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:454: void > _patchClass(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:465: void > _tryPatchGetter(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:468: if > (!origin.isAbstractField()) { > I'd share this check between tryPatchGetter and tryPatchSetter. Normally, I > wouldn't, but in this case, the audience is comprised exclusively of Dart > library maintainers, so we don't need fine granularity of messages. > > This made me think of something related: perhaps we should make sure that all > these messages are all prefixed with PATCH_. This allows us to group these error > codes together and let users of the API know that these errors are internal and > additional documentation shouldn't be necessary. Also, translation should be a > low priority. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:494: void > _tryPatchSetter(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:523: void > _tryPatchConstructor(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:540: void > _tryPatchFunction(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:557: void > _patchFunction(leg.DiagnosticListener listener, > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... > sdk/lib/_internal/compiler/implementation/patch_parser.dart:584: bool > _isPatchElement(Element element) { > I'd like to see a unit test of this method. > > https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... > File tests/compiler/dart2js/patch_test.dart (right): > > https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... > tests/compiler/dart2js/patch_test.dart:395: """ > Why a multiline string? > > https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... > tests/compiler/dart2js/patch_test.dart:405: compiler.errors[0].message.kind == > MessageKind.PATCH_NON_EXISTING); > I don't think you're testing the positions. This applies to all the tests below. > Let's try to think of a way we can make it easy to test positions. > > https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... > tests/compiler/dart2js/patch_test.dart:445: compiler.errors[0].message.kind == > MessageKind.PATCH_NONPATCHABLE); > This, and many tests below, do not test that you get the informational message > that is generated. You don't like private method. Some do. In any case, the wish for a unit test on each private method seems only to be trickered by the fact the methods are private and therefore cannot be tested directly. The updated testcase tests every situation and does so by using the current framework for patches, and thus tests that the code is actually exercised through the normal code path. I see no added benefit of a unittest of each method. Also I would imagine that the test case for setting up a call to these methods would be more complex than the methods themselves, which defeats the purpose of unittesting.
I think the problem is that the patch parser is written in a way that makes it really hard to create a patch element without applying it. This leads to bad decisions, such as, avoiding unit testing basic functionality and attempting to encapsulate the code in an API that is really complex. I'll shortly upload a patch that demonstrates how to make the patch parser more testable.
On 2013/01/23 10:30:32, ahe wrote: > I'll shortly upload a patch that demonstrates how to make the patch parser more > testable. Here it is: https://codereview.chromium.org/12047041/
PTAL I don't find direct test of patchElement very useful since it bypasses the most crucial part: Testing that we connect the patch with the intended origin. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:393: void _patchElement(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by all methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:402: } On 2013/01/11 11:46:12, ngeoffray wrote: > indentation Done. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:403: if (!(patch.isClass() || On 2013/01/21 10:53:35, ahe wrote: > Let me suggest a shorter alternative version of this method. > > First we check that the patch has certain properties, then we check that the > origin has almost the same properties. And then we switch on these same > properties to report errors once again. > > I would have written the code like this: > > if (patch.isClass()) { > tryPatchClass(listener, origin, patch); > } else if (patch.isGetter()) { > tryPatchGetter(listener, origin, patch); > } else if (patch.isSetter()) { > tryPatchSetter(listener, origin, patch); > } else if (patch.isConstructor()) { > tryPatchConstructor(listener, origin, patch); > } else if(patch.isFunction()) { > tryPatchFunction(listener, origin, patch); > } else { > listener.reportMessage( > listener.spanFromSpannable(patch), > leg.MessageKind.PATCH_NONPATCHABLE.error(), > api.Diagnostic.ERROR); > } > > This code is equivalent, but rather than three "switches", you have only one (so > this is slightly faster). In addition, you don't need any asserts to check what > you just checked (assert on line 432 is rechecking lines 403-406). This makes > the code easier to extend to handle a new kind of element. Almost done. Test for patchable origin must remain. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:437: void _tryPatchClass(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by several methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:454: void _patchClass(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by several methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:461: origin.patch = patch; On 2013/01/11 11:46:12, ngeoffray wrote: > Use setPatch to hide implementation details? Added a TODO. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:465: void _tryPatchGetter(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by some methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:468: if (!origin.isAbstractField()) { On 2013/01/21 10:53:35, ahe wrote: > I'd share this check between tryPatchGetter and tryPatchSetter. Normally, I > wouldn't, but in this case, the audience is comprised exclusively of Dart > library maintainers, so we don't need fine granularity of messages. > > This made me think of something related: perhaps we should make sure that all > these messages are all prefixed with PATCH_. This allows us to group these error > codes together and let users of the API know that these errors are internal and > additional documentation shouldn't be necessary. Also, translation should be a > low priority. The granularity has already been made and it takes a lot of work to merge the two. Moved all patch message together and ensure the PATCH_ prefix. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:494: void _tryPatchSetter(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by a method in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:523: void _tryPatchConstructor(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by a method in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:540: void _tryPatchFunction(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by some methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:557: void _patchFunction(leg.DiagnosticListener listener, On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. It's tested by some methods in patch_test. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:581: patch.origin = origin; On 2013/01/11 11:46:12, ngeoffray wrote: > Add a setOrigin? Added a TODO. https://codereview.chromium.org/11864010/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/patch_parser.dart:584: bool _isPatchElement(Element element) { On 2013/01/21 10:53:35, ahe wrote: > I'd like to see a unit test of this method. Added a TODO. https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... File tests/compiler/dart2js/patch_test.dart (right): https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:48: bool isRegular: false}) { On 2013/01/11 11:46:12, ngeoffray wrote: > What's 'isRegular'? Maybe add a comment. A regular member is one that is neither patched nor a patch nor declared in a patch library. The terminology is explained in 'patch_parser.dart'. https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:395: """ On 2013/01/21 10:53:35, ahe wrote: > Why a multiline string? Expanded with the missing class declaration. https://codereview.chromium.org/11864010/diff/1/tests/compiler/dart2js/patch_... tests/compiler/dart2js/patch_test.dart:445: compiler.errors[0].message.kind == MessageKind.PATCH_NONPATCHABLE); On 2013/01/21 10:53:35, ahe wrote: > This, and many tests below, do not test that you get the informational message > that is generated. Generally a good idea, but not easy currently.
Ping
https://codereview.chromium.org/11864010/diff/10001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/11864010/diff/10001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/patch_parser.dart:592: // TODO(lrn): More checks needed if we introduce metadata for real. I think you can remove TODO or file a bug to replace patch modifiers with annotations.
LGTM! https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); I don't understand why you're renaming this parameter.
https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); On 2013/02/04 13:20:22, ahe wrote: > I don't understand why you're renaming this parameter. I think this change comes from a rebase. I do think that calling it 'spannable' instead of the archaic 'node' is better, though.
https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); On 2013/02/05 08:48:42, Johnni Winther wrote: > On 2013/02/04 13:20:22, ahe wrote: > > I don't understand why you're renaming this parameter. > > I think this change comes from a rebase. I do think that calling it 'spannable' > instead of the archaic 'node' is better, though. I disagree. I don't know why you're calling the name "node" archaic. It was not an accident I chose that name, and I have used it in other situations since then.
https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); On 2013/02/05 08:51:19, ahe wrote: > On 2013/02/05 08:48:42, Johnni Winther wrote: > > On 2013/02/04 13:20:22, ahe wrote: > > > I don't understand why you're renaming this parameter. > > > > I think this change comes from a rebase. I do think that calling it > 'spannable' > > instead of the archaic 'node' is better, though. > > I disagree. I don't know why you're calling the name "node" archaic. It was not > an accident I chose that name, and I have used it in other situations since > then. It indicates that a spannable is always a node which is not true. When the signature read SourceSpan spanFromNode(Node node, [Uri uri]) it was true, but its not anymore; that's what I mean by 'archaic'.
https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); On 2013/02/05 08:51:19, ahe wrote: > On 2013/02/05 08:48:42, Johnni Winther wrote: > > On 2013/02/04 13:20:22, ahe wrote: > > > I don't understand why you're renaming this parameter. > > > > I think this change comes from a rebase. I do think that calling it > 'spannable' > > instead of the archaic 'node' is better, though. > > I disagree. I don't know why you're calling the name "node" archaic. It was not > an accident I chose that name, and I have used it in other situations since > then. So what is the good reason for still calling it 'node'?
https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart (left): https://codereview.chromium.org/11864010/diff/15001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart:19: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); On 2013/02/05 13:24:12, Johnni Winther wrote: > On 2013/02/05 08:51:19, ahe wrote: > > On 2013/02/05 08:48:42, Johnni Winther wrote: > > > On 2013/02/04 13:20:22, ahe wrote: > > > > I don't understand why you're renaming this parameter. > > > > > > I think this change comes from a rebase. I do think that calling it > > 'spannable' > > > instead of the archaic 'node' is better, though. > > > > I disagree. I don't know why you're calling the name "node" archaic. It was > not > > an accident I chose that name, and I have used it in other situations since > > then. > > So what is the good reason for still calling it 'node'? I don't know if it is a particular good reason, but I dislike made up words that ends with "-able". If you think the word "node" is confusing, I think you should change it, but remember these other places: sdk/lib/_internal/compiler/implementation/compiler.dart: SourceSpan spanFromSpannable(Spannable node, [Uri uri]) { sdk/lib/_internal/compiler/implementation/compiler.dart: void reportErrorCode(Spannable node, MessageKind errorCode, sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart: SourceSpan spanFromSpannable(Spannable node, [Uri uri]); sdk/lib/_internal/compiler/implementation/diagnostic_listener.dart: void reportErrorCode(Spannable node, MessageKind errorCode, [Map arguments]); sdk/lib/_internal/compiler/implementation/util/util.dart: final Spannable node; |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
