Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280033002:
Move Parser::Declare to Scope. (Closed)
DescriptionMove Parser::Declare to Scope.
Parser::Declare has a lot of Scope-related logic inside; especially it
does Lookup in Scope. Scope should be the class which knows how to
declare variables in different kinds of Scopes, not Parser.
BUG=
Committed: https://crrev.com/ee7dc92f9ec397671ea9a61d7fdac5315abef214
Cr-Commit-Position: refs/heads/master@{#38979}
Patch Set 1 #Patch Set 2 : first complete version #Patch Set 3 : rebased #Patch Set 4 : beautification #
Total comments: 2
Patch Set 5 : less invasive #Patch Set 6 : . #
Total comments: 2
Patch Set 7 : code review (verwaest@) #
Total comments: 7
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move Parser::Declare to Scope. -- WIP -- DO NOT COMMIT -- Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= ========== to ========== Move Parser::Declare to Scope. Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= ==========
The CQ bit was checked by marja@chromium.org to run a CQ dry run
marja@chromium.org changed reviewers: + verwaest@google.com
ptal https://codereview.chromium.org/2280033002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:784: declaration->position(), declaration->proxy()->end_position(), Here the positions are different than they used to be; Parser::ReportMessage just took the position from Scanner. But this should be at least as good as that solution, right? https://codereview.chromium.org/2280033002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:809: decls_.Add(declaration, zone()); Inlined Scope::AddDeclaration here.
marja@chromium.org changed reviewers: + verwaest@chromium.org - verwaest@google.com
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2280033002/diff/100001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:721: Variable* var = NULL; nullptr
thanks https://codereview.chromium.org/2280033002/diff/100001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:721: Variable* var = NULL; On 2016/08/29 12:01:24, Toon Verwaest wrote: > nullptr Done.
The CQ bit was checked by marja@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/2280033002/#ps120001 (title: "code review (verwaest@)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move Parser::Declare to Scope. Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= ========== to ========== Move Parser::Declare to Scope. Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move Parser::Declare to Scope. Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= ========== to ========== Move Parser::Declare to Scope. Parser::Declare has a lot of Scope-related logic inside; especially it does Lookup in Scope. Scope should be the class which knows how to declare variables in different kinds of Scopes, not Parser. BUG= Committed: https://crrev.com/ee7dc92f9ec397671ea9a61d7fdac5315abef214 Cr-Commit-Position: refs/heads/master@{#38979} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ee7dc92f9ec397671ea9a61d7fdac5315abef214 Cr-Commit-Position: refs/heads/master@{#38979}
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
lgtm, I like the overall plan here, just a few comments (mostly on comments, I suppose I could just send a followup) https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc#newc... src/ast/scopes.cc:764: // The name was declared in this scope before; check for conflicting This comment seems out of place and out of date. Maybe I'll just send out a CL moving it around/deleting parts of it. https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc#newc... src/ast/scopes.cc:778: // In harmony we treat re-declarations as early errors. See This comment is now out of place (it belongs in the caller). https://codereview.chromium.org/2280033002/diff/120001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2280033002/diff/120001/src/parsing/parser.cc#... src/parsing/parser.cc:1690: } I'd prefer if there was an explicit "return nullptr" here, otherwise we're depending on the implementation details of Scope::DeclareVariable() to make sure this function returns null (and that the use counter isn't hit in the error case).
Message was sent while issue was closed.
Thanks for comments; follow-up CL here: https://codereview.chromium.org/2292223004 https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc#newc... src/ast/scopes.cc:764: // The name was declared in this scope before; check for conflicting On 2016/08/30 18:58:32, adamk wrote: > This comment seems out of place and out of date. Maybe I'll just send out a CL > moving it around/deleting parts of it. Ok, then I won't do anything for it.. https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc#newc... src/ast/scopes.cc:778: // In harmony we treat re-declarations as early errors. See On 2016/08/30 18:58:32, adamk wrote: > This comment is now out of place (it belongs in the caller). Hmm, but the decision to set *ok = false is done here! https://codereview.chromium.org/2280033002/diff/120001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2280033002/diff/120001/src/parsing/parser.cc#... src/parsing/parser.cc:1690: } On 2016/08/30 18:58:32, adamk wrote: > I'd prefer if there was an explicit "return nullptr" here, otherwise we're > depending on the implementation details of Scope::DeclareVariable() to make sure > this function returns null (and that the use counter isn't hit in the error > case). Ah, true, it's definitely good to return nullptr here.
Message was sent while issue was closed.
https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2280033002/diff/120001/src/ast/scopes.cc#newc... src/ast/scopes.cc:778: // In harmony we treat re-declarations as early errors. See On 2016/08/31 07:44:53, marja wrote: > On 2016/08/30 18:58:32, adamk wrote: > > This comment is now out of place (it belongs in the caller). > > Hmm, but the decision to set *ok = false is done here! Ah, I hadn't thought about it that way. I was thinking of this referring to the old if/else logic (I was confusing it with the old runtime error for redeclaration). This is fine. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
