Traitify ParserBase and move functions there.

Traitify ParserBase and move functions there.

The long-term goal is to move all recursive descent functions from Parser and
PreParser into ParserBase, but first they need to be unified.

Notes:
- The functions moved in this CL: ParseIdentifier, ParseIdentifierName,
ParseIdentifierNameOrGetOrSet, ParseIdentifierOrStrictReservedWord.
- IOW, this CL removes Parser::ParseIdentifier and PreParser::ParseIdentifier
  and adds ParserBase::ParseIdentifier, etc.
- Error reporting used to require virtual funcs; now error reporting is moved to
  the Traits too, and ParserBase no longer needs to be virtual.
- I had to move PreParser::Identifier out of the PreParser class, because
otherwise PreParserTraits cannot use it in a typedef.

BUG=v8:3126
LOG=N
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=19230

[Michael Starzinger, 2014-02-10 13:28:03]

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc#newcode607
src/parser.cc:607: new ParserTraits(this)),
If we make the ParserBase inherit from ParserTraits/PreParserTraits then we
wouldn't need to allocate any additional object, everything needed to parse
would be embedded in the Parser objects.

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc#newcode1098
src/parser.cc:1098: traits_->ReportMessage("module_export_undefined",
Likewise calling ReportMessage() wouldn't need an explicit receiver if
ParserBase inherits from ParserTraits.

https://codereview.chromium.org/149913006/diff/100001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/149913006/diff/100001/src/parser.h#newcode411
src/parser.h:411: class Parser;
nit: Can we group this forward declaration together with the previous one and
alpha-sort?

https://codereview.chromium.org/149913006/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/149913006/diff/100001/src/preparser.h#newcode52
src/preparser.h:52: delete traits_;
It feels dangerous to have the ownership of the Traits object be transferred to
the parser. Could we avoid that?

[marja, 2014-02-10 15:02:07]

mstarzinger: Thanks for having a look!

Do you like this version better?

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc#newcode607
src/parser.cc:607: new ParserTraits(this)),
On 2014/02/10 13:28:03, Michael Starzinger wrote:
> If we make the ParserBase inherit from ParserTraits/PreParserTraits then we
> wouldn't need to allocate any additional object, everything needed to parse
> would be embedded in the Parser objects.

Done.

https://codereview.chromium.org/149913006/diff/100001/src/parser.cc#newcode1098
src/parser.cc:1098: traits_->ReportMessage("module_export_undefined",
On 2014/02/10 13:28:03, Michael Starzinger wrote:
> Likewise calling ReportMessage() wouldn't need an explicit receiver if
> ParserBase inherits from ParserTraits.

Done.

https://codereview.chromium.org/149913006/diff/100001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/149913006/diff/100001/src/parser.h#newcode411
src/parser.h:411: class Parser;
On 2014/02/10 13:28:03, Michael Starzinger wrote:
> nit: Can we group this forward declaration together with the previous one and
> alpha-sort?

Done.

https://codereview.chromium.org/149913006/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/149913006/diff/100001/src/preparser.h#newcode52
src/preparser.h:52: delete traits_;
On 2014/02/10 13:28:03, Michael Starzinger wrote:
> It feels dangerous to have the ownership of the Traits object be transferred
to
> the parser. Could we avoid that?

Done. but this is not pretty either:

1) Functions in preparser.h cannot just say GetSymbol() etc. ("there are no
arguments to nnn that depend on a template parameter, so a declaration of mmm
must be available")

2) Functions in parser.cc can say GetSymbol() etc., but all functions will
eventually move to ParserBase, so, it's not much use.

3) ReportMessage exists on multiple levels of the class hierarchy with different
parameters, so, to get the right version called, I need to qualify with the
class.

[Michael Starzinger, 2014-02-10 15:25:11]

LGTM, just some nits left.

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc#newcode4084
src/parser.cc:4084: if (!eval_args_error_log.IsValid() &&
nit: Should fit into one line again now.

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc#newcode4435
src/parser.cc:4435: if (lhs != NULL && !lhs->is_this() &&
nit: Likewise.

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode53
src/preparser.h:53: ~ParserBase() {}
nit: Shouldn't be necessary to declare at all now.

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode216
src/preparser.h:216: typename Traits::IdentifierType ParseIdentifier(
Puzzler of the day: Can a typename nested within a class be forward declared?
Would spare us from repeating the nasty "typename" everywhere. Chances are slim
I guess, but lets try. :)

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode307
src/preparser.h:307: // it is used) are generally omitted.
This whole block of comments mainly applies to the PreParser class, can we move
it down to right before the PreParser class?

[marja, 2014-02-10 15:32:01]

Thanks for review!

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc#newcode4084
src/parser.cc:4084: if (!eval_args_error_log.IsValid() &&
On 2014/02/10 15:25:12, Michael Starzinger wrote:
> nit: Should fit into one line again now.

(Was already done by a later patch set.)

https://codereview.chromium.org/149913006/diff/150001/src/parser.cc#newcode4435
src/parser.cc:4435: if (lhs != NULL && !lhs->is_this() &&
On 2014/02/10 15:25:12, Michael Starzinger wrote:
> nit: Likewise.

(Was already done by a later patch set.)

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode53
src/preparser.h:53: ~ParserBase() {}
On 2014/02/10 15:25:12, Michael Starzinger wrote:
> nit: Shouldn't be necessary to declare at all now.

Done.

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode216
src/preparser.h:216: typename Traits::IdentifierType ParseIdentifier(
On 2014/02/10 15:25:12, Michael Starzinger wrote:
> Puzzler of the day: Can a typename nested within a class be forward declared?
> Would spare us from repeating the nasty "typename" everywhere. Chances are
slim
> I guess, but lets try. :)

Nope :)

https://codereview.chromium.org/149913006/diff/150001/src/preparser.h#newcode307
src/preparser.h:307: // it is used) are generally omitted.
On 2014/02/10 15:25:12, Michael Starzinger wrote:
> This whole block of comments mainly applies to the PreParser class, can we
move
> it down to right before the PreParser class?

Done.

[marja, 2014-02-10 15:35:55]

Committed patchset #8 manually as r19230 (presubmit successful).