[parser] Give preparser and parser independent loggers

[parser] Give preparser and parser independent loggers

This
- removes the ParserRecorder base class,
- devirtualizes the LogFunction and LogMessage functions,
- reuses the SingletonLogger for all preparser calls

In a subsequent step the preparser should probably log directly to the CompleteParserRecorder rather than indirectly through the singleton logger...

BUG=

Committed: https://crrev.com/32105d214da5750506d11e6b292145e7cacbcd3b
Cr-Commit-Position: refs/heads/master@{#40803}

[Toon Verwaest, 2016-11-04 14:10:26]

verwaest@chromium.org changed reviewers:
+ vogelheim@chromium.org

[Toon Verwaest, 2016-11-04 14:10:27]

ptal

[marja, 2016-11-04 14:47:06]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2016-11-04 14:47:07]

(Didn't yet read through all of this, posting a mid-review comment)

https://codereview.chromium.org/2474393003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2474393003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2791: SingletonLogger* logger =
reusable_preparser_->logger();
It's kinda meh that we now don't pass the logger in
ParseFunctionWithPreParser... so this makes the data flow from preparser to
parser even less explicit.

Otoh, the number of params and especially the fact that there might be an error
and then we need different data than in the non-error case make it pretty
impractical to just pass int* and stuff*.

So I don't have anything better to suggest here...

[Toon Verwaest, 2016-11-04 14:47:08]

Patchset #3 (id:40001) has been deleted

[Toon Verwaest, 2016-11-04 14:49:13]

Yeah I guess in the long run we should pass in the outer logger and int* and let
the preparser fill everything in.

Apart from that, always recreating a singleton logger doesn't seem particularly
useful...

[marja, 2016-11-04 14:59:57]

lgtm %

https://codereview.chromium.org/2474393003/diff/20001/src/parsing/preparse-da...
File src/parsing/preparse-data.h (right):

https://codereview.chromium.org/2474393003/diff/20001/src/parsing/preparse-da...
src/parsing/preparse-data.h:49: class SingletonLogger final {
Now the names make less sense than before, how about PreParserLogger and
ParserLogger? Or something descriptive?

[Toon Verwaest, 2016-11-04 15:08:40]

Thanks, done!

[Toon Verwaest, 2016-11-04 15:10:11]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-04 15:10:12]

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/2474393003/#ps60002
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-04 15:10:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2016-11-04 15:17:15]

lgtm

(Only looked briefly, though, since Marja already reviewed and she knows the
code better anyhow.)

[commit-bot: I haz the power, 2016-11-04 15:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 15:28:08]

Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11963)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Toon Verwaest, 2016-11-07 12:24:18]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 12:24:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-11-07 12:25:11]

Dropping the reentrance check since it's already failing with arrow functions.
Over time we want this to become reentrant anyway, and it doesn't add very much
value at this point given that the entry-point is quite specific for switching
from the parser to the preparser.

[commit-bot: I haz the power, 2016-11-07 12:30:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 12:30:03]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[Toon Verwaest, 2016-11-07 12:41:40]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 12:41:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2016-11-07 12:43:59]

lgtm

(One nitpick, which isn't even related to your change. Feel free to ignore...)

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc#n...
src/parsing/parser.cc:721: if (produce_cached_parse_data()) {
nitpick (and actually unrelated to your changes, so feel free to ignore...):

I was trying to understand when log_ would be == nullptr and when not... The
code here is correct and should always leave the function with log_ == nullptr,
but I think this would be slightly easier to understand if written like this:

if (produce_cached_parse_data() && result != NULL) {
  *info->cached_data() = logger.GetScriptData();
}
log_ = nullptr;
return result;

[vogelheim, 2016-11-07 12:43:59]

lgtm

(One nitpick, which isn't even related to your change. Feel free to ignore...)

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc#n...
src/parsing/parser.cc:721: if (produce_cached_parse_data()) {
nitpick (and actually unrelated to your changes, so feel free to ignore...):

I was trying to understand when log_ would be == nullptr and when not... The
code here is correct and should always leave the function with log_ == nullptr,
but I think this would be slightly easier to understand if written like this:

if (produce_cached_parse_data() && result != NULL) {
  *info->cached_data() = logger.GetScriptData();
}
log_ = nullptr;
return result;

[Toon Verwaest, 2016-11-07 12:55:34]

Addressed

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2474393003/diff/90001/src/parsing/parser.cc#n...
src/parsing/parser.cc:721: if (produce_cached_parse_data()) {
On 2016/11/07 12:43:59, vogelheim wrote:
> nitpick (and actually unrelated to your changes, so feel free to ignore...):
> 
> I was trying to understand when log_ would be == nullptr and when not... The
> code here is correct and should always leave the function with log_ ==
nullptr,
> but I think this would be slightly easier to understand if written like this:
> 
> if (produce_cached_parse_data() && result != NULL) {
>   *info->cached_data() = logger.GetScriptData();
> }
> log_ = nullptr;
> return result;
> 

Done.

[Toon Verwaest, 2016-11-07 12:57:57]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-07 12:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2474393003/#ps130001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-11-07 12:58:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 13:23:07]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2016-11-17 22:24:41]

Description was changed from

==========
[parser] Give preparser and parser independent loggers

This
- removes the ParserRecorder base class,
- devirtualizes the LogFunction and LogMessage functions,
- reuses the SingletonLogger for all preparser calls

In a subsequent step the preparser should probably log directly to the
CompleteParserRecorder rather than indirectly through the singleton logger...

BUG=
==========

to

==========
[parser] Give preparser and parser independent loggers

This
- removes the ParserRecorder base class,
- devirtualizes the LogFunction and LogMessage functions,
- reuses the SingletonLogger for all preparser calls

In a subsequent step the preparser should probably log directly to the
CompleteParserRecorder rather than indirectly through the singleton logger...

BUG=

Committed: https://crrev.com/32105d214da5750506d11e6b292145e7cacbcd3b
Cr-Commit-Position: refs/heads/master@{#40803}
==========

[commit-bot: I haz the power, 2016-11-17 22:24:42]

Patchset 7 (id:??) landed as
https://crrev.com/32105d214da5750506d11e6b292145e7cacbcd3b
Cr-Commit-Position: refs/heads/master@{#40803}