Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351233002:
[parser] Refactor of Parse*Statement*, part 8 (Closed)
Description[parser] Refactor of Parse*Statement*, part 8
This patch moves the following parsing method to ParserBase:
- ParseForStatement
R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/51b6a3d11b225b474600837b75b901a3f08c5d51
Cr-Commit-Position: refs/heads/master@{#39587}
Patch Set 1 : Just moving code (broken) #Patch Set 2 : The real patch #
Total comments: 23
Patch Set 3 : The real patch (fixed base) #Patch Set 4 : Change after reviewers' comments #
Messages
Total messages: 22 (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...
I know this is a bit ugly but it's arguably not uglier than the existing Parser::ParseForStatement. Given the time I found reasonable to devote right now, I think it's near the best I can do. It can be re-refactored, but this will require a more general rewriting. Most probably, we'll have to do this anyway when we start experimenting with more implementations (e.g., when the pre-parser starts declaring variables).
A bit surprisingly, I think there are some ways in which this is cleaner than what we had before (mostly just because things are split into more helper functions). Only two comments: one is that ForInfo looks like it could be passed by const reference everywhere (none of the helpers need to mutate it). And then one style nit below. Other than that, lgtm if it passes tests (given the complexity I'd appreciate a pass from marja as well). https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5080: body_block_and_each_variable.first->statements()->Add(body, zone()); Please create a variable for body_block, it'll make the rest of this code a little more readable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Initial set of comments, I'll continue tomorrow. Curiously, when I select "view" in the files in the patch set, I get the same view as in when viewing patch set deltas (so, it always shows the Parser::ParseForStatement on the left side). That's weird. Maybe a bug in code review. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5016: for_info.bound_names_are_lexical = bound_names_are_lexical is now redundant, right? Maybe we shouldn't have it (as the information is available in for_info.parsing_result), so that there's no fear that for_info.bound_names_are_lexical and for_info.parsing_result.descriptor.mode disagree. If it makes the code clearer, there can be a bound_names_are_lexical helper variable wherever it's needed (this is quite far away from the use..) https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5052: auto loop = I was thinking about the use of "auto" here (and below)... it's not clear from the context what type it is (some factory funcs, especially in the PreParser, return generic types of Statements, some of them return specialized Statements... so it's not possible to say just after reading this line what type auto suppresses. However, does the use of auto here allow us to get away with fewer typedefs in the traits? (A stupid question.) Any other benefits I'm not seeing? https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1998: // Special case for legacy for (var ... = ... in ...) Could this comment also explain what it is rewritten to (like the comment below about for-in/of - that is very illustrative). https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2017: } else { Style nit: else { } around the return is unnecessary and doesn't make the code any clearer. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2073: if (name != Style nit: I'd combine these two nested if's: if (name != ... && for_info->bound_names.Contains(name)) { error } https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2338: NULL, for_info->bound_names.length() + 3, true, kNoSourcePosition); Nit: pls update to nullptr while anyway modifying this line
The approach LG - it's definitely clearer than the previous version, and you don't need to fix all the world's problems in this patch. But I think there's something wrong w/ the patch / diffs: 1) did you set a weird baseline to the branch? 2) Why no changes in preparser.cc? https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5032: for_info.mode == ForEachStatement::ITERATE || Random whining unrelated to this CL: Why do we need to have ITERATE and ENUMERATE, so that everybody reading the code needs to remember that ITERATE is "for of" and ENUMERATE is "for in". I have hard time remembering which is which. IMO it would be clearer if we used the terms "ForIn" and "ForOf". In addition, bugs where somebody accidentally uses the wrong one would look more obvious (now they're quite hard to spot). For example, this code would read: for_info.mode == ForEachStatement.FOR_IN || ... || allow_harmony_for_in(), so it's clear that they belong together. If you agree, maybe a follow-up CL? https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5080: body_block_and_each_variable.first->statements()->Add(body, zone()); On 2016/09/20 16:34:31, adamk wrote: > Please create a variable for body_block, it'll make the rest of this code a > little more readable. Why do we need the pair anyway, it looks like we're always using either .first or .second? https://codereview.chromium.org/2351233002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:456: In addition, shouldn't you remove PreParser::ParseForStatement?
Sorry about the mess with the patch's base, for some reason, the 2nd patchset was based on the 1st one. This was fixed. Marja, you may want to take another look. Concerning Adam's comment about ForInfo* vs. const ForInfo&, I kept the former for DesugarBindingInForEachStatement, which seems to need it for passing the names list to PatternRewriter::DeclareAndInitializeVariables (which may change it, as far as I can see). For all other methods, I switched to the latter. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5016: for_info.bound_names_are_lexical = On 2016/09/20 19:36:30, marja wrote: > bound_names_are_lexical is now redundant, right? Maybe we shouldn't have it (as > the information is available in for_info.parsing_result), so that there's no > fear that for_info.bound_names_are_lexical and > for_info.parsing_result.descriptor.mode disagree. > > If it makes the code clearer, there can be a bound_names_are_lexical helper > variable wherever it's needed (this is quite far away from the use..) Yes. I'm removing it from ForInfo but I'm keeping it as a local variable here, to avoid unnecessary calls to IsLexicalVariableMode. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5032: for_info.mode == ForEachStatement::ITERATE || On 2016/09/21 08:23:21, marja wrote: > Random whining unrelated to this CL: Why do we need to have ITERATE and > ENUMERATE, so that everybody reading the code needs to remember that ITERATE is > "for of" and ENUMERATE is "for in". I have hard time remembering which is which. > IMO it would be clearer if we used the terms "ForIn" and "ForOf". In addition, > bugs where somebody accidentally uses the wrong one would look more obvious (now > they're quite hard to spot). > > For example, this code would read: for_info.mode == ForEachStatement.FOR_IN || > ... || allow_harmony_for_in(), so it's clear that they belong together. > > > If you agree, maybe a follow-up CL? I agree. Also, it seems that this is used only locally in the parser. There are 4+9 occurrences of ENUMERATE+ITERATE. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5052: auto loop = On 2016/09/20 19:36:30, marja wrote: > I was thinking about the use of "auto" here (and below)... it's not clear from > the context what type it is (some factory funcs, especially in the PreParser, > return generic types of Statements, some of them return specialized > Statements... so it's not possible to say just after reading this line what type > auto suppresses. > > However, does the use of auto here allow us to get away with fewer typedefs in > the traits? (A stupid question.) Any other benefits I'm not seeing? Yes, that's precisely what it does. (BTW, it's used in the same way in other places too, e.g., while and do-while loops.) In the preparser, loop is a PreParserStatement; in the parser, it's a ForEachStatement*. We would need a typedef such as Types::ForEachStatementT if we did not to use "auto" here. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5080: body_block_and_each_variable.first->statements()->Add(body, zone()); On 2016/09/21 08:23:21, marja wrote: > On 2016/09/20 16:34:31, adamk wrote: > > Please create a variable for body_block, it'll make the rest of this code a > > little more readable. > > Why do we need the pair anyway, it looks like we're always using either .first > or .second? After offline discussion, I'm removing the pair and introducing two out parameters to DesugarBindingInForEachStatement. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5080: body_block_and_each_variable.first->statements()->Add(body, zone()); On 2016/09/20 16:34:31, adamk wrote: > Please create a variable for body_block, it'll make the rest of this code a > little more readable. See comment below. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:1998: // Special case for legacy for (var ... = ... in ...) On 2016/09/20 19:36:30, marja wrote: > Could this comment also explain what it is rewritten to (like the comment below > about for-in/of - that is very illustrative). Done, with a bit of reverse-engineering. You may want to see the comment I added, in case I'm missing something. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2017: } else { On 2016/09/20 19:36:30, marja wrote: > Style nit: else { } around the return is unnecessary and doesn't make the code > any clearer. Done. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2073: if (name != On 2016/09/20 19:36:30, marja wrote: > Style nit: I'd combine these two nested if's: > > if (name != ... && for_info->bound_names.Contains(name)) { > error > } Done. I think the part "name != ... dot_catch_string" may be redundant. Is there any way the for loop can declare such a name? https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2338: NULL, for_info->bound_names.length() + 3, true, kNoSourcePosition); On 2016/09/20 19:36:30, marja wrote: > Nit: pls update to nullptr while anyway modifying this line Done. Here and elsewhere. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/preparser.h... src/parsing/preparser.h:456: On 2016/09/21 08:23:21, marja wrote: > In addition, shouldn't you remove PreParser::ParseForStatement? Yes, obviously. Done.
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.
lgtm https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:5052: auto loop = Ok, then it's fine. https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2073: if (name != I was thinking the same, wondering if it's redundant... Idk. Probably yes. Didn't investigate.
https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2351233002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:2073: if (name != On 2016/09/21 10:31:42, marja wrote: > I was thinking the same, wondering if it's redundant... Idk. Probably yes. > Didn't investigate. OK, let's play it safe and leave it.
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 Link to the patchset: https://codereview.chromium.org/2351233002/#ps60001 (title: "Change after reviewers' comments")
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 8 This patch moves the following parsing method to ParserBase: - ParseForStatement R=adamk@chromium.org, marja@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor of Parse*Statement*, part 8 This patch moves the following parsing method to ParserBase: - ParseForStatement R=adamk@chromium.org, marja@chromium.org BUG= LOG=N Committed: https://crrev.com/51b6a3d11b225b474600837b75b901a3f08c5d51 Cr-Commit-Position: refs/heads/master@{#39587} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/51b6a3d11b225b474600837b75b901a3f08c5d51 Cr-Commit-Position: refs/heads/master@{#39587} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
