Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699423002:
Implement AstBuilder support for top level variables and function literals. (Closed)
|
Created:
3 years, 10 months ago by Paul Berry Modified:
3 years, 10 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplement AstBuilder support for top level variables and function literals.
R=ahe@google.com
Committed: https://github.com/dart-lang/sdk/commit/9b04857653a5ae35b1a5b65d47b3d5b439304a34
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comment changes #
Total comments: 2
Messages
Total messages: 13 (2 generated)
paulberry@google.com changed reviewers: + ahe@google.com, scheglov@google.com
lgtm I like your comments in listener.dart. I'll try to use this idea if when add new events. All my comments below are informative, no action items for this CL. https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right): https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:319: // reached, node would always be a VariableDeclaration. Good idea. I've filed issue https://github.com/dart-lang/sdk/issues/28833. However, now that I've filed that bug, I realize I can implement this event without changing dart2js, so I should #justdoit. https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:944: // variables. This is a bit tricky when it comes to handling initializers in constructors. So we should make sure that this class and BodyBuilder shares most of this logic. https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:971: // TODO(paulberry,ahe): the parser needs to pass the var/const/final keyword Those should be available in beginToken. Another option is building a different object to represent modifiers.
https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right): https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:319: // reached, node would always be a VariableDeclaration. On 2017/02/20 09:04:20, ahe wrote: > Good idea. I've filed issue https://github.com/dart-lang/sdk/issues/28833. > > However, now that I've filed that bug, I realize I can implement this event > without changing dart2js, so I should #justdoit. Thanks for doing that! Now that the new event is in, I would like to try my hand at cleaning up the kludge, so I've changed it to just "TODO(paulberry)" :) https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:944: // variables. On 2017/02/20 09:04:20, ahe wrote: > This is a bit tricky when it comes to handling initializers in constructors. So > we should make sure that this class and BodyBuilder shares most of this logic. Good point. I've updated the TODO comment to make a note of this. https://codereview.chromium.org/2699423002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:971: // TODO(paulberry,ahe): the parser needs to pass the var/const/final keyword On 2017/02/20 09:04:20, ahe wrote: > Those should be available in beginToken. Another option is building a different > object to represent modifiers. Oh, ok. I misread the parser code and thought "beginToken" included metadata. I've updated the TODO comment and I will fix in a follow-up CL.
Description was changed from ========== Implement AstBuilder support for top level variables and function literals. ========== to ========== Implement AstBuilder support for top level variables and function literals. R=ahe@google.com Committed: https://github.com/dart-lang/sdk/commit/9b04857653a5ae35b1a5b65d47b3d5b439304a34 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 9b04857653a5ae35b1a5b65d47b3d5b439304a34 (presubmit successful).
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/parser/listener.dart (right): https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field initializer Seems inconsistent. Maybe variable in both or field in both? Also the specification does not call these "fields", it call them "top-level variables" or "library variables".
Message was sent while issue was closed.
FYI https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/parser/listener.dart (right): https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field initializer On 2017/02/20 20:10:36, scheglov wrote: > Seems inconsistent. > Maybe variable in both or field in both? > Also the specification does not call these "fields", it call them "top-level > variables" or "library variables". I think it would make sense to go go through this file and ensure consistent naming throughout. I've created a spreadsheet where we can do that: https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... While we're doing that, I think it would make sense to stay consistent with what's already in this file. So I think it should be top-level field, not variable.
Message was sent while issue was closed.
On 2017/02/21 09:10:24, ahe wrote: > FYI > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > File pkg/front_end/lib/src/fasta/parser/listener.dart (right): > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field initializer > On 2017/02/20 20:10:36, scheglov wrote: > > Seems inconsistent. > > Maybe variable in both or field in both? > > Also the specification does not call these "fields", it call them "top-level > > variables" or "library variables". > > I think it would make sense to go go through this file and ensure consistent > naming throughout. I've created a spreadsheet where we can do that: > https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... > > While we're doing that, I think it would make sense to stay consistent with > what's already in this file. So I think it should be top-level field, not > variable. I don't mind putting up with naming inconsistencies in the name of reducing code churn--nobody wants to waste time renaming things when we could be fixing bugs and implementing features. But if we're going to go to the trouble to make the names in this file consistent with each other, we should go ahead and make them consistent with the spec too. One of my biggest sources of confusion when I started working on analyzer was that its nomenclature was (for historical reasons) inconsistent with the spec; I would hate for front_end to have the same problem for new contributors.
Message was sent while issue was closed.
On 2017/02/21 13:57:29, Paul Berry wrote: > On 2017/02/21 09:10:24, ahe wrote: > > FYI > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > File pkg/front_end/lib/src/fasta/parser/listener.dart (right): > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field > initializer > > On 2017/02/20 20:10:36, scheglov wrote: > > > Seems inconsistent. > > > Maybe variable in both or field in both? > > > Also the specification does not call these "fields", it call them "top-level > > > variables" or "library variables". > > > > I think it would make sense to go go through this file and ensure consistent > > naming throughout. I've created a spreadsheet where we can do that: > > > https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... > > > > While we're doing that, I think it would make sense to stay consistent with > > what's already in this file. So I think it should be top-level field, not > > variable. > > I don't mind putting up with naming inconsistencies in the name of reducing code > churn--nobody wants to waste time renaming things when we could be fixing bugs > and implementing features. But if we're going to go to the trouble to make the > names in this file consistent with each other, we should go ahead and make them > consistent with the spec too. One of my biggest sources of confusion when I > started working on analyzer was that its nomenclature was (for historical > reasons) inconsistent with the spec; I would hate for front_end to have the same > problem for new contributors. I agree with a minor caveat: sometimes I think it makes sense to deviate from the specification, but one should have a good reason for doing so. But this is an exception.
Message was sent while issue was closed.
On 2017/02/21 14:03:24, ahe wrote: > On 2017/02/21 13:57:29, Paul Berry wrote: > > On 2017/02/21 09:10:24, ahe wrote: > > > FYI > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > File pkg/front_end/lib/src/fasta/parser/listener.dart (right): > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field > > initializer > > > On 2017/02/20 20:10:36, scheglov wrote: > > > > Seems inconsistent. > > > > Maybe variable in both or field in both? > > > > Also the specification does not call these "fields", it call them > "top-level > > > > variables" or "library variables". > > > > > > I think it would make sense to go go through this file and ensure consistent > > > naming throughout. I've created a spreadsheet where we can do that: > > > > > > https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... > > > > > > While we're doing that, I think it would make sense to stay consistent with > > > what's already in this file. So I think it should be top-level field, not > > > variable. > > > > I don't mind putting up with naming inconsistencies in the name of reducing > code > > churn--nobody wants to waste time renaming things when we could be fixing bugs > > and implementing features. But if we're going to go to the trouble to make > the > > names in this file consistent with each other, we should go ahead and make > them > > consistent with the spec too. One of my biggest sources of confusion when I > > started working on analyzer was that its nomenclature was (for historical > > reasons) inconsistent with the spec; I would hate for front_end to have the > same > > problem for new contributors. > > I agree with a minor caveat: sometimes I think it makes sense to deviate from > the specification, but one should have a good reason for doing so. But this is > an exception. Clarifying question: By "this is an exception" which do you mean: a. In this case one does not need a good reason for deviating from the specification (!) b. This is a case where it does make sense to deviate from the specification, and the good reason for doing so is... c. This is a case where we should follow the spec, because there's no good reason for deviating from it.
Message was sent while issue was closed.
On 2017/02/21 17:06:11, Paul Berry wrote: > On 2017/02/21 14:03:24, ahe wrote: > > On 2017/02/21 13:57:29, Paul Berry wrote: > > > On 2017/02/21 09:10:24, ahe wrote: > > > > FYI > > > > > > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > > File pkg/front_end/lib/src/fasta/parser/listener.dart (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > > pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field > > > initializer > > > > On 2017/02/20 20:10:36, scheglov wrote: > > > > > Seems inconsistent. > > > > > Maybe variable in both or field in both? > > > > > Also the specification does not call these "fields", it call them > > "top-level > > > > > variables" or "library variables". > > > > > > > > I think it would make sense to go go through this file and ensure > consistent > > > > naming throughout. I've created a spreadsheet where we can do that: > > > > > > > > > > https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... > > > > > > > > While we're doing that, I think it would make sense to stay consistent > with > > > > what's already in this file. So I think it should be top-level field, not > > > > variable. > > > > > > I don't mind putting up with naming inconsistencies in the name of reducing > > code > > > churn--nobody wants to waste time renaming things when we could be fixing > bugs > > > and implementing features. But if we're going to go to the trouble to make > > the > > > names in this file consistent with each other, we should go ahead and make > > them > > > consistent with the spec too. One of my biggest sources of confusion when I > > > started working on analyzer was that its nomenclature was (for historical > > > reasons) inconsistent with the spec; I would hate for front_end to have the > > same > > > problem for new contributors. > > > > I agree with a minor caveat: sometimes I think it makes sense to deviate from > > the specification, but one should have a good reason for doing so. But this is > > an exception. > > Clarifying question: By "this is an exception" which do you mean: > a. In this case one does not need a good reason for deviating from the > specification (!) > b. This is a case where it does make sense to deviate from the specification, > and the good reason for doing so is... > c. This is a case where we should follow the spec, because there's no good > reason for deviating from it. I meant that in general not following the specification should be an exception. In this concrete case, I think using top-level variable makes sense and I've already added the suggestion to the spreadsheet. Email is hard :-)
Message was sent while issue was closed.
On 2017/02/21 17:47:28, ahe wrote: > On 2017/02/21 17:06:11, Paul Berry wrote: > > On 2017/02/21 14:03:24, ahe wrote: > > > On 2017/02/21 13:57:29, Paul Berry wrote: > > > > On 2017/02/21 09:10:24, ahe wrote: > > > > > FYI > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > > > File pkg/front_end/lib/src/fasta/parser/listener.dart (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2699423002/diff/20001/pkg/front_end/lib/src/f... > > > > > pkg/front_end/lib/src/fasta/parser/listener.dart:561: /// - Field > > > > initializer > > > > > On 2017/02/20 20:10:36, scheglov wrote: > > > > > > Seems inconsistent. > > > > > > Maybe variable in both or field in both? > > > > > > Also the specification does not call these "fields", it call them > > > "top-level > > > > > > variables" or "library variables". > > > > > > > > > > I think it would make sense to go go through this file and ensure > > consistent > > > > > naming throughout. I've created a spreadsheet where we can do that: > > > > > > > > > > > > > > > https://docs.google.com/a/google.com/spreadsheets/d/1fyBErAV168aGJ2Dajn3aA1cd... > > > > > > > > > > While we're doing that, I think it would make sense to stay consistent > > with > > > > > what's already in this file. So I think it should be top-level field, > not > > > > > variable. > > > > > > > > I don't mind putting up with naming inconsistencies in the name of > reducing > > > code > > > > churn--nobody wants to waste time renaming things when we could be fixing > > bugs > > > > and implementing features. But if we're going to go to the trouble to > make > > > the > > > > names in this file consistent with each other, we should go ahead and make > > > them > > > > consistent with the spec too. One of my biggest sources of confusion when > I > > > > started working on analyzer was that its nomenclature was (for historical > > > > reasons) inconsistent with the spec; I would hate for front_end to have > the > > > same > > > > problem for new contributors. > > > > > > I agree with a minor caveat: sometimes I think it makes sense to deviate > from > > > the specification, but one should have a good reason for doing so. But this > is > > > an exception. > > > > Clarifying question: By "this is an exception" which do you mean: > > a. In this case one does not need a good reason for deviating from the > > specification (!) > > b. This is a case where it does make sense to deviate from the specification, > > and the good reason for doing so is... > > c. This is a case where we should follow the spec, because there's no good > > reason for deviating from it. > > I meant that in general not following the specification should be an exception. > > In this concrete case, I think using top-level variable makes sense and I've > already added the suggestion to the spreadsheet. > > Email is hard :-) Thanks for the clarification! I'm glad I asked :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
