Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289663002:
[parser] Hide expression classifiers in parser implementation (Closed)
Description[parser] Hide expression classifiers in parser implementation
This patch removes the explicit classifier parameters from all
parsing methods and makes expression classifiers implicit in
the (pre)parser's implementation. In this way, the implementation
is simplified and a proper stack of classifiers is enforced.
R=adamk@chromium.org,littledan@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/9d818bea602b7d56aaaffc5cb64b49b98890332c
Cr-Commit-Position: refs/heads/master@{#39068}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simpler and better version #
Total comments: 9
Patch Set 3 : Change after reviewers' comments #Patch Set 4 : Rebase #
Messages
Total messages: 30 (16 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...
https://codereview.chromium.org/2289663002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2289663002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1255: // TODO(nikolaos): This looks horrible, let's see about it... Well, maybe it's not so bad after all, especially if it is renamed into something better and made implementation-specific. https://codereview.chromium.org/2289663002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1279: bool accumulating_; This is needed just for the case of class declarations, which should temporarily work in their own stack.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The perf bots seem to produce the regular random results for this... :-)
Description was changed from ========== [parser] Hide expression classifiers in parser implementation This patch is NOT TO BE LANDED yet !!! This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, a proper stack of classifiers is enforced. TBR=adamk@chromium.org,littledan@chromium.org BUG= LOG=N ========== to ========== [parser] Hide expression classifiers in parser implementation This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, the implementation is simplified and a proper stack of classifiers is enforced. R=adamk@chromium.org,littledan@chromium.org BUG= LOG=N ==========
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 Looks great. I'm somewhat surprised that we seem to have already been following this stack discipline properly already, but I don't see any actual semantic changes in this patch. This is a really good refactoring that both makes the code cleaner and seems to pave the way for cheaper ExpressionClassifiers for the preparser.
On 2016/08/29 20:27:01, Dan Ehrenberg wrote: > lgtm > > Looks great. I'm somewhat surprised that we seem to have already been following > this stack discipline properly already, but I don't see any actual semantic > changes in this patch. This is a really good refactoring that both makes the > code cleaner and seems to pave the way for cheaper ExpressionClassifiers for the > preparser. Since the patch that moved the errors out of classifiers and into (practically) a stack, we had been forced to follow the stack discipline for classifiers, otherwise it just wouldn't work.
https://codereview.chromium.org/2289663002/diff/20001/src/parsing/expression-... File src/parsing/expression-classifier.h (left): https://codereview.chromium.org/2289663002/diff/20001/src/parsing/expression-... src/parsing/expression-classifier.h:85: } We probably should add, at the bottom of the class: DISALLOW_COPY_AND_ASSIGN(ExpressionClassifier);
This mostly looks good, but the naming question around ParseExpressionNoWrap is something I'd like to resolve before landing. https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1067: // classifier and calls the RewriteNonPattern if parsing is successful. Nit: s/the RewriteNonPattern/RewriteNonPattern/ https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1070: // new expression classifier; it uses the top-level classifier instead. I think this comment should explain a bit more of what this method is used for, not just how it works: it's for parsing the cover grammar in an ambiguous context. https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1071: ExpressionT ParseExpressionNoWrap(bool accept_IN, bool* ok); "ParseExpressionNoWrap" isn't at all clear to me. I'd go for "ParseAndClassifyExpression" instead.
https://codereview.chromium.org/2289663002/diff/20001/src/parsing/expression-... File src/parsing/expression-classifier.h (left): https://codereview.chromium.org/2289663002/diff/20001/src/parsing/expression-... src/parsing/expression-classifier.h:85: } On 2016/08/30 09:30:15, nickie wrote: > We probably should add, at the bottom of the class: > > DISALLOW_COPY_AND_ASSIGN(ExpressionClassifier); Done. https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1067: // classifier and calls the RewriteNonPattern if parsing is successful. On 2016/08/30 22:24:52, adamk wrote: > Nit: s/the RewriteNonPattern/RewriteNonPattern/ Done. https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1070: // new expression classifier; it uses the top-level classifier instead. On 2016/08/30 22:24:52, adamk wrote: > I think this comment should explain a bit more of what this method is used for, > not just how it works: it's for parsing the cover grammar in an ambiguous > context. Done. It now reads: // This method wraps the parsing of the expression inside a new expression // classifier and calls RewriteNonPattern if parsing is successful. // It should be used whenever we're parsing an expression that will be // used as a non-pattern (i.e., in most cases). V8_INLINE ExpressionT ParseExpression(bool accept_IN, bool* ok); https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1071: ExpressionT ParseExpressionNoWrap(bool accept_IN, bool* ok); On 2016/08/30 22:24:52, adamk wrote: > "ParseExpressionNoWrap" isn't at all clear to me. I'd go for > "ParseAndClassifyExpression" instead. Before giving this a different name, I tried a few variants that just overloaded ParseExpression. We could do that, if you think it's better, e.g., add a "bool cover" parameter that would be true if we're not sure whether we're parsing a pattern or a non-pattern (i.e., when using the cover grammar). But I couldn't find an elegant and efficient (when inlined) way to do this without defining at least one method with a new name (not directly used). Ultimately I thought it would be clearer if the parameter was spared and the new name was actually used. However, ParseExpressionNoWrap is not a particularly good name. But I'm afraid "ParseAndClassifyExpression" is not good either, because it suggests that this method is going to do the classification, which is just the opposite from the truth. How about ParseExpressionCover? And a fat comment explaining what this is about, like: // This method does not wrap the parsing of the expression inside a // new expression classifier; it uses the top-level classifier instead. // It should be used whenever we're parsing something with the "cover" // grammar that recognizes both patterns and non-patterns (which roughly // corresponds to what's inside the parentheses generated by the symbol // "CoverParenthesizedExpressionAndArrowParameterList" in the ES 2017 // specification). ExpressionT ParseExpressionCover(bool accept_IN, bool* ok);
lgtm (once the new patch is uploaded, with the discussed name change; I don't see a patch set 3 at the moment) https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2289663002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1071: ExpressionT ParseExpressionNoWrap(bool accept_IN, bool* ok); On 2016/08/31 08:19:53, nickie wrote: > On 2016/08/30 22:24:52, adamk wrote: > > "ParseExpressionNoWrap" isn't at all clear to me. I'd go for > > "ParseAndClassifyExpression" instead. > > Before giving this a different name, I tried a few variants that just overloaded > ParseExpression. We could do that, if you think it's better, e.g., add a "bool > cover" parameter that would be true if we're not sure whether we're parsing a > pattern or a non-pattern (i.e., when using the cover grammar). But I couldn't > find an elegant and efficient (when inlined) way to do this without defining at > least one method with a new name (not directly used). Ultimately I thought it > would be clearer if the parameter was spared and the new name was actually used. > > However, ParseExpressionNoWrap is not a particularly good name. But I'm afraid > "ParseAndClassifyExpression" is not good either, because it suggests that this > method is going to do the classification, which is just the opposite from the > truth. > > How about ParseExpressionCover? And a fat comment explaining what this is > about, like: > > // This method does not wrap the parsing of the expression inside a > // new expression classifier; it uses the top-level classifier instead. > // It should be used whenever we're parsing something with the "cover" > // grammar that recognizes both patterns and non-patterns (which roughly > // corresponds to what's inside the parentheses generated by the symbol > // "CoverParenthesizedExpressionAndArrowParameterList" in the ES 2017 > // specification). > ExpressionT ParseExpressionCover(bool accept_IN, bool* ok); I like the comment (and the reference to CoverParenthesizedExpressionAndArrowParameterList). And I agree that a separate method is nicer than a parameter. How about ParseExpressionCoverGrammar? If you feel it's redundant to include "Grammar" in the name of a parser method, I'd be ok with falling back to ParseExpressionCover.
On 2016/08/31 18:15:22, adamk wrote: > lgtm (once the new patch is uploaded, with the discussed name change; I don't > see a patch set 3 at the moment) ParseExpressionCoverGrammar it will be. I'm uploading and landing the patch set now.
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2289663002/#ps40001 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11913) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7987)
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2289663002/#ps60001 (title: "Rebase")
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 ========== [parser] Hide expression classifiers in parser implementation This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, the implementation is simplified and a proper stack of classifiers is enforced. R=adamk@chromium.org,littledan@chromium.org BUG= LOG=N ========== to ========== [parser] Hide expression classifiers in parser implementation This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, the implementation is simplified and a proper stack of classifiers is enforced. R=adamk@chromium.org,littledan@chromium.org BUG= LOG=N ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Hide expression classifiers in parser implementation This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, the implementation is simplified and a proper stack of classifiers is enforced. R=adamk@chromium.org,littledan@chromium.org BUG= LOG=N ========== to ========== [parser] Hide expression classifiers in parser implementation This patch removes the explicit classifier parameters from all parsing methods and makes expression classifiers implicit in the (pre)parser's implementation. In this way, the implementation is simplified and a proper stack of classifiers is enforced. R=adamk@chromium.org,littledan@chromium.org BUG= LOG=N Committed: https://crrev.com/9d818bea602b7d56aaaffc5cb64b49b98890332c Cr-Commit-Position: refs/heads/master@{#39068} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9d818bea602b7d56aaaffc5cb64b49b98890332c Cr-Commit-Position: refs/heads/master@{#39068} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
