Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339453002:
[parser] Refactor of Parse*Statement*, part 7 (Closed)
Description[parser] Refactor of Parse*Statement*, part 7
This patch moves the following parsing method to ParserBase:
- ParseTryStatement
R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/7b2297c90d65163d93fc6a65fd7ec4d5d2456ea6
Cr-Commit-Position: refs/heads/master@{#39467}
Patch Set 1 : Just moving code (broken) #Patch Set 2 : The real patch #
Total comments: 9
Patch Set 3 : Change after reviewers' comments #
Total comments: 2
Patch Set 4 : Change in comment #
Messages
Total messages: 26 (11 generated)
The CQ bit was checked by nikolaos@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.
https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.h File src/parsing/parser.h (left): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.h#ol... src/parsing/parser.h:377: Expression* MakeCatchContext(Handle<String> id, VariableProxy* value); Also, this declaration is redundant (there's no definition for this method). I'll delete it before landing.
Initial comments (will have a more detailed look later): Q: does this unify the languages accepted by Parser / PreParser too, or is this orthogonal? I was wondering if it would be applicable to add test-parsing unification tests (those which basically test that Parser and PreParser produce the same errors). https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1795: if (catch_info->pattern != nullptr) { Would it be possible to push more of this work to ParserBase, so that it would then call impl()->DeclareAndInitializeVariables? Otoh, if PreParser will never declare variables still, maybe it's not a good idea. I'm conflicted (because I don't know if we're moving towards the world where PreParser declares variables or not).
This was more tedious to review than the previous ones, but I think I checked every line now... https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1788: void Parser::RewriteCatchPattern(CatchInfo* catch_info, bool* ok) { Wouldn't "DeclareCatchVariable" be a better name? https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:953: bool* ok) { Now this code is in both Parser and PreParser; what's the reason for not writing it in a unified manner?
On 2016/09/14 09:58:09, marja wrote: > Initial comments (will have a more detailed look later): > > Q: does this unify the languages accepted by Parser / PreParser too, or is this > orthogonal? I was wondering if it would be applicable to add test-parsing > unification tests (those which basically test that Parser and PreParser produce > the same errors). Apart from things related to variable declarations (e.g., in the catch scope), the parser and the preparser should be consistent w.r.t. the try statement. I'm not sure I understand the question, but I'm not claiming that this patch changes the preparser's behavior to catch any more errors. https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1795: if (catch_info->pattern != nullptr) { On 2016/09/14 09:58:09, marja wrote: > Would it be possible to push more of this work to ParserBase, so that it would > then call impl()->DeclareAndInitializeVariables? > > Otoh, if PreParser will never declare variables still, maybe it's not a good > idea. I'm conflicted (because I don't know if we're moving towards the world > where PreParser declares variables or not). I suppose it can, which will probably make this method entirely redundant. Let's hear what Adam says about this and/or discuss it offline.
https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1788: void Parser::RewriteCatchPattern(CatchInfo* catch_info, bool* ok) { On 2016/09/14 10:06:59, marja wrote: > Wouldn't "DeclareCatchVariable" be a better name? It's not only declaring, it's also rewriting, introducing the init_block in the case of a destructuring pattern. But I agree that the word "declare" is missing from the method's name. https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:953: bool* ok) { On 2016/09/14 10:06:59, marja wrote: > Now this code is in both Parser and PreParser; what's the reason for not writing > it in a unified manner? Well, for some weird reason, in the two implementations this code is guarded by different conditions. It won't be easy to keep the same behavior and make the code look handsome. If it comes to changing the behavior, I'm not sure what exactly this does.
https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:953: bool* ok) { On 2016/09/14 10:19:09, nickie wrote: > On 2016/09/14 10:06:59, marja wrote: > > Now this code is in both Parser and PreParser; what's the reason for not > writing > > it in a unified manner? > > Well, for some weird reason, in the two implementations this code is guarded by > different conditions. It won't be easy to keep the same behavior and make the > code look handsome. If it comes to changing the behavior, I'm not sure what > exactly this does. I think that should get figured out before landing this... If it turns out that there was really a behavioral difference, i.e., Parser was producing an error where PreParser wasn't, or the other way around, I think that should be fixed as a side product of this unification. That would also be a good opportunity to add the corresponding test-parsing unification test. So let's see: In PreParser: if (FLAG_harmony_explicit_tailcalls && catch_block_exists && tail_call_expressions_in_catch_block.has_explicit_tail_calls()) { ... In Parser: if (FLAG_harmony_explicit_tailcalls && tail_call_expressions_in_catch_block.has_explicit_tail_calls()) { I think they are the same; there's no way that tail_call_expressions_in_catch_block has something and catch_block_exists wouldn't be set to true.
Unless Adam has something else, there's just the thing with method Parser::RewriteCatchPattern (see comment): its name and what it should do. This boils down to whether we want to implement preparser support for declaring variables in this CL (just support, not the actual thing, of course). In that case, the declaration of the variables should move from this method to the parser base. I'm inclined to keep the behavior as it is and probably rename the method to reflect the fact that it also declares variables, but I have no strong opinion about this. https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2339453002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:953: bool* ok) { On 2016/09/14 11:00:27, marja wrote: > On 2016/09/14 10:19:09, nickie wrote: > > On 2016/09/14 10:06:59, marja wrote: > > > Now this code is in both Parser and PreParser; what's the reason for not > > writing > > > it in a unified manner? > > > > Well, for some weird reason, in the two implementations this code is guarded > by > > different conditions. It won't be easy to keep the same behavior and make the > > code look handsome. If it comes to changing the behavior, I'm not sure what > > exactly this does. > > I think that should get figured out before landing this... > > If it turns out that there was really a behavioral difference, i.e., Parser was > producing an error where PreParser wasn't, or the other way around, I think that > should be fixed as a side product of this unification. That would also be a good > opportunity to add the corresponding test-parsing unification test. > > So let's see: > > In PreParser: > > if (FLAG_harmony_explicit_tailcalls && catch_block_exists && > tail_call_expressions_in_catch_block.has_explicit_tail_calls()) { > > ... > > In Parser: > > if (FLAG_harmony_explicit_tailcalls && > tail_call_expressions_in_catch_block.has_explicit_tail_calls()) { > > > I think they are the same; there's no way that > tail_call_expressions_in_catch_block has something and catch_block_exists > wouldn't be set to true. Done. (As discussed offline.)
lgtm (We can always move the catch variable declaration closer to base if we need it... for now, let's not, since it's not sure it'll be needed.)
The CQ bit was checked by nikolaos@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.
One question, otherwise this works for me (it's pretty ugly, but I don't have good suggestions until we do something different for handling destructuring overall). https://codereview.chromium.org/2339453002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2339453002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:4866: // "hack" for keeping track of arrow function names. If/when What does this have to do with arrow function names? I added this if-statement recently to remove a call to RemoveUnresolved() (which we did if ParsePrimaryExpression returned a VariableProxy).
Oh, and lgtm, since my question is just about a TODO
It is ugly, but wait until you see the for statement... :-) https://codereview.chromium.org/2339453002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2339453002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:4866: // "hack" for keeping track of arrow function names. If/when On 2016/09/15 18:32:07, adamk wrote: > What does this have to do with arrow function names? > > I added this if-statement recently to remove a call to RemoveUnresolved() (which > we did if ParsePrimaryExpression returned a VariableProxy). My TODO comment refers to lines 4868-4875. If you use ParsePrimaryExpression in both cases, ExpressionFromIdentifier is called. One of the side effects that you get then is the one that you mention (about the unresolved). I must admit I didn't notice that. The second side effect, however, which causes several tests to break, is a fni_->PushVariableName(name) which seems to change the function name for arrow functions. That's what I meant. I will rephrase my TODO comment before landing.
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2339453002/#ps60001 (title: "Change in comment")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Refactor of Parse*Statement*, part 7 This patch moves the following parsing method to ParserBase: - ParseTryStatement R=adamk@chromium.org, marja@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor of Parse*Statement*, part 7 This patch moves the following parsing method to ParserBase: - ParseTryStatement R=adamk@chromium.org, marja@chromium.org BUG= LOG=N Committed: https://crrev.com/7b2297c90d65163d93fc6a65fd7ec4d5d2456ea6 Cr-Commit-Position: refs/heads/master@{#39467} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7b2297c90d65163d93fc6a65fd7ec4d5d2456ea6 Cr-Commit-Position: refs/heads/master@{#39467} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
