Replace is_reference_error bool argument with ParseErrorType enum

Replace is_reference_error bool argument with ParseErrorType enum

This avoids accidental coercion-to-bool when calling ReportMessage()
in the parser (e.g., from pointer types), and as a bonus makes callsites
easier to read.

Committed: https://crrev.com/82368b17e9314ebea142fb56aaf08f4f938c69dd
Cr-Commit-Position: refs/heads/master@{#26788}

[adamk, 2015-02-19 21:32:39]

adamk@chromium.org changed reviewers:
+ arv@chromium.org, marja@chromium.org

[arv (Not doing code reviews), 2015-02-19 21:41:59]

LGTM

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

https://codereview.chromium.org/939303002/diff/1/src/parser.cc#newcode648
src/parser.cc:648: parser_->pending_error_is_reference_error_ = error_type ==
kReferenceError;
Maybe change pending_error_is_reference_error_ to pending_error_type_?

https://codereview.chromium.org/939303002/diff/1/src/preparse-data.cc
File src/preparse-data.cc (right):

https://codereview.chromium.org/939303002/diff/1/src/preparse-data.cc#newcode45
src/preparse-data.cc:45: function_store_.Add(error_type == kReferenceError ? 1 :
0);
Can you just Add(error_type) here?

https://codereview.chromium.org/939303002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/939303002/diff/1/src/preparser.h#newcode2987
src/preparser.h:2987: "unexpected_token", "ILLEGAL", kSyntaxError);
Maybe remove kSyntaxError here and other places since that is the default?

[adamk, 2015-02-19 22:03:22]

New patchsets have been uploaded after l-g-t-m from arv@chromium.org

[adamk, 2015-02-19 22:03:31]

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

https://codereview.chromium.org/939303002/diff/1/src/parser.cc#newcode648
src/parser.cc:648: parser_->pending_error_is_reference_error_ = error_type ==
kReferenceError;
On 2015/02/19 21:41:59, arv wrote:
> Maybe change pending_error_is_reference_error_ to pending_error_type_?

Done.

https://codereview.chromium.org/939303002/diff/1/src/preparse-data.cc
File src/preparse-data.cc (right):

https://codereview.chromium.org/939303002/diff/1/src/preparse-data.cc#newcode45
src/preparse-data.cc:45: function_store_.Add(error_type == kReferenceError ? 1 :
0);
On 2015/02/19 21:41:59, arv wrote:
> Can you just Add(error_type) here?

I'd rather not depend on the numeric value of the enum in a serialization.

On another topic, it looks like this serialized data is only used by tests?

https://codereview.chromium.org/939303002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/939303002/diff/1/src/preparser.h#newcode2987
src/preparser.h:2987: "unexpected_token", "ILLEGAL", kSyntaxError);
On 2015/02/19 21:41:59, arv wrote:
> Maybe remove kSyntaxError here and other places since that is the default?

I'd like to leave that for a followup. With all the overloads right now it's
possible these arguments are there to avoid ambiguity.

[arv (Not doing code reviews), 2015-02-19 23:02:43]

Still LGTM

[marja, 2015-02-20 09:11:47]

lgtm

https://codereview.chromium.org/939303002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/939303002/diff/20001/src/parser.cc#newcode4295
src/parser.cc:4295: pending_error_type_ == kReferenceError
Can you add a static assert here that there are only those 2 types of errors (so
that compilation fails here if somebody adds a third type)?

https://codereview.chromium.org/939303002/diff/20001/src/preparse-data.cc
File src/preparse-data.cc (right):

https://codereview.chromium.org/939303002/diff/20001/src/preparse-data.cc#new...
src/preparse-data.cc:45: function_store_.Add(error_type == kReferenceError ? 1 :
0);
Ditto, though, re: previous discussion, I don't see why this version works any
better (or is more future-proof) than relying on the numeric values.

[adamk, 2015-02-20 20:22:47]

New patchsets have been uploaded after l-g-t-m from
arv@chromium.org,marja@chromium.org

[adamk, 2015-02-20 20:23:04]

https://codereview.chromium.org/939303002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/939303002/diff/20001/src/parser.cc#newcode4295
src/parser.cc:4295: pending_error_type_ == kReferenceError
On 2015/02/20 09:11:47, marja wrote:
> Can you add a static assert here that there are only those 2 types of errors
(so
> that compilation fails here if somebody adds a third type)?

Added a switch() statement, which is the best way I know to statically assert
which values an enum can take.

https://codereview.chromium.org/939303002/diff/20001/src/preparse-data.cc
File src/preparse-data.cc (right):

https://codereview.chromium.org/939303002/diff/20001/src/preparse-data.cc#new...
src/preparse-data.cc:45: function_store_.Add(error_type == kReferenceError ? 1 :
0);
On 2015/02/20 09:11:47, marja wrote:
> Ditto, though, re: previous discussion, I don't see why this version works any
> better (or is more future-proof) than relying on the numeric values.

Eh, I'll use the enum value and add a comment to globals.h.

[adamk, 2015-02-20 20:27:02]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-02-20 20:27:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/939303002/40001

[commit-bot: I haz the power, 2015-02-20 20:27:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/939303002/40001

[commit-bot: I haz the power, 2015-02-20 21:19:44]

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

[commit-bot: I haz the power, 2015-02-20 21:19:45]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-02-20 21:19:45]

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

[commit-bot: I haz the power, 2015-02-20 21:19:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-20 21:20:08]

Patchset 3 (id:??) landed as
https://crrev.com/82368b17e9314ebea142fb56aaf08f4f938c69dd
Cr-Commit-Position: refs/heads/master@{#26788}