Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179423002:
[parser] Refactor parser and preparser traits (Closed)
Description[parser] Refactor parser and preparser traits
This patch refactors the traits objects, used by the parser and the
preparser, so that they contain the same set of methods, with the same
signatures.
R=adamk@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/8dd835c2c6f7315070e552cc823e7ff768ac6c4d
Cr-Commit-Position: refs/heads/master@{#38736}
Patch Set 1 #Patch Set 2 : Refactor symbol preparser methods #Patch Set 3 : Rebase #
Total comments: 6
Patch Set 4 : Change after reviewers' comments #Patch Set 5 : More changes after reviewers' comments #
Dependent Patchsets: Messages
Total messages: 37 (26 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...
Description was changed from ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N ==========
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 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.
For some reason (most probably my bad), this CL was never sent for review before I left for vacation. I just rebased it. It's the first step in my pre/parser traits refactoring.
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.
adamk@chromium.org changed reviewers: + marja@chromium.org
I won't get a chance to look at this in detail today, but I'll give it a go tomorrow. But adding marja to the reviewers, as these traits classes were largely her creation.
LGTM % comments This is generally sensemaking, though, if you're going to merge Parser with ParseTraits and PreParser with PreparserTraits and use the curiously recurring template pattern (class Parser : public ParserBase<Parser>), it's less important which functions are in now Parser and which in ParserTraits. https://codereview.chromium.org/2179423002/diff/40001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2179423002/diff/40001/src/parsing/parser.h#ne... src/parsing/parser.h:508: static const AstRawString* EmptyIdentifier() { return NULL; } If you're anyway modifying this line, you could change NULL to nullptr. (We change them gradually whenever modifying the code and use nullptr in new code.) https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.cc File src/parsing/preparser.cc (right): https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.c... src/parsing/preparser.cc:61: switch (scanner->current_token()) { This change is pretty surprising in the context of this CL. Is this faster than the previous one? https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.h... src/parsing/preparser.h:967: void SetFunctionNameFromPropertyName(PreParserExpression property, I think this change is unnecessary, the parameter names were left out to indicate they are dummy unused parameters and this function is required just to implement the API.
On 2016/08/18 08:36:15, marja wrote:
> This is generally sensemaking, though, if you're going to merge Parser with
> ParseTraits and PreParser with PreparserTraits and use the curiously recurring
> template pattern (class Parser : public ParserBase<Parser>), it's less
important
> which functions are in now Parser and which in ParserTraits.
Unfortunately, it's a bit more complicated, as there is some cyclic dependency.
The refactoring (described in a design doc that I'll make publicly available
shortly, when the related CL is uploaded) will look roughly like:
template <typename Impl>
class ParserBaseTraits;
template <typename Impl>
class ParserBase : public ParserBaseTraits<Impl> { ... };
class Parser;
template <>
class ParserBaseTraits<Parser> { ... };
class Parser : public ParserBase<Parser> { ... };
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/parser.h
File src/parsing/parser.h (right):
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/parser.h#ne...
src/parsing/parser.h:508: static const AstRawString* EmptyIdentifier() { return
NULL; }
On 2016/08/18 08:36:15, marja wrote:
> If you're anyway modifying this line, you could change NULL to nullptr. (We
> change them gradually whenever modifying the code and use nullptr in new
code.)
Done.
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.c...
src/parsing/preparser.cc:61: switch (scanner->current_token()) {
On 2016/08/18 08:36:16, marja wrote:
> This change is pretty surprising in the context of this CL. Is this faster
than
> the previous one?
You're right, this change is rather unrelated, but I couldn't resist... :-)
I think a switch is preferable here. The definition of current_token is inline
and pretty simple, so the optimizer should only evaluate it once, but
(officially) the method is not even flagged as "const" there. I don't expect it
to be really faster (although in principle, this could save 5-6 comparisons most
of the times that GetSymbol is called). It's definitely not slower.
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.h
File src/parsing/preparser.h (right):
https://codereview.chromium.org/2179423002/diff/40001/src/parsing/preparser.h...
src/parsing/preparser.h:967: void
SetFunctionNameFromPropertyName(PreParserExpression property,
On 2016/08/18 08:36:16, marja wrote:
> I think this change is unnecessary, the parameter names were left out to
> indicate they are dummy unused parameters and this function is required just
to
> implement the API.
I have no strong opinion about this; I just wanted to make the method headers
consistent between the two files.
However, the codestyle [1] says:
"""
Parameter names may be omitted only if the parameter is unused and its purpose
is obvious.
...
Unused parameters that might not be obvious should comment out the variable name
in the function definition:
...
void Circle::Rotate(double /*radians*/) {}
"""
Would you rather have the parameter names in comments to stress that they are
not actually used?
[1]
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
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 don't have a strong opinion about the unused parameter thing. The commented out version is not very much used in our code base iirc. But I have a stronger opinion about the if-switch change, I'd prefer to have that in another CL!
Iow, if the coding style discourages the version I like, fair enough; the other two versions are both equally good imo.
On 2016/08/18 09:18:16, marja wrote: > I don't have a strong opinion about the unused parameter thing. The commented > out version is not very much used in our code base iirc. > > But I have a stronger opinion about the if-switch change, I'd prefer to have > that in another CL! Sure, then I'm leaving the parameters and I'm removing the if-switch from this CL. Then I'm waiting for Adam's comments.
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 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 Sad we have to touch so many lines of code, but I guess we've already had plenty of git-blame disruption over the last year with the file splits/moves.
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org Link to the patchset: https://codereview.chromium.org/2179423002/#ps80001 (title: "More changes 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.
Description was changed from ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor parser and preparser traits This patch refactors the traits objects, used by the parser and the preparser, so that they contain the same set of methods, with the same signatures. R=adamk@chromium.org BUG= LOG=N Committed: https://crrev.com/8dd835c2c6f7315070e552cc823e7ff768ac6c4d Cr-Commit-Position: refs/heads/master@{#38736} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8dd835c2c6f7315070e552cc823e7ff768ac6c4d Cr-Commit-Position: refs/heads/master@{#38736} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
