Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by jwolfe Modified:
4 years, 7 months ago Reviewers:
caitp (gmail), Dan Ehrenberg CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionIn parallel to the strict octal check that would reject `012` in strict mode, this patch collects UseCounters for `089` in strict mode. The spec says this should be an error, but this patch does not report it as such.
BUG=v8:4973
LOG=y
Committed: https://crrev.com/d0b6686c14339bd5d0aeaf610705c7ed85393e1f
Cr-Commit-Position: refs/heads/master@{#36221}
Patch Set 1 #
Total comments: 6
Patch Set 2 : tidy up my changes to Scanner::ScanNumber #Patch Set 3 : Turn error into UseCounter #Patch Set 4 : update instructions for making corresponding changes in chromium #
Total comments: 1
Patch Set 5 : don't forget histograms.xml #Patch Set 6 : fix uninitialized field. guard against null #
Total comments: 1
Created: 4 years, 7 months ago
Messages
Total messages: 27 (11 generated)
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#newc... src/parsing/scanner.cc:1034: decimal_that_looks_like_octal = true; '8' and '9' are decidedly not octal digits --- why are we saying this looks octal? Just confused about what problem this solves
Sorry, I think I get it, you want to prevent reporting the error if a series of octal digits are followed by a decimal digit, right? Anyways, there are some nits here https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (left): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#oldc... src/parsing/scanner.cc:990: int start_pos = source_pos(); // For reporting octal positions. This change seems not to be needed, since it's irrelevant when the number doesn't begin with 0 https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#newc... src/parsing/scanner.cc:1067: if (decimal_that_looks_like_octal) `decimal_that_looks_like_octal` is always false unless the `c0_ == '0'` branch is taken on line 991 --- these should be safe to get rid of, AFAIK?
The variable "decimal_that_looks_like_octal" could definitely use a better name. I just really don't like the name "NonOctalDecimalIntegerLiteral", because that sounds like it would apply to every decimal integer literal that is not octal (e.g. "1").
https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (left): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#oldc... src/parsing/scanner.cc:990: int start_pos = source_pos(); // For reporting octal positions. On 2016/05/05 18:25:02, caitp wrote: > This change seems not to be needed, since it's irrelevant when the number > doesn't begin with 0 I can leave the local variable uninitialized until this line. I just need to declare it out where the later code can see it. I would have loved to have dealt with `octal_pos_` inside the `c0_ == '0'` block, but I didn't know where the end of the token was going to be yet. https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#newc... src/parsing/scanner.cc:1067: if (decimal_that_looks_like_octal) On 2016/05/05 18:25:02, caitp wrote: > `decimal_that_looks_like_octal` is always false unless the `c0_ == '0'` branch > is taken on line 991 --- these should be safe to get rid of, AFAIK? I was surprised by needing two different checks in this function. This check is for `09`, and the one on line 1107 is for `019`. I don't know why `019` isn't reported as a SMI token by this function. Perhaps it's a mistake?
caitpotter88@gmail.com changed reviewers: + littledan@chromium.org
+littledan for frontend review
https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1948403002/diff/1/src/parsing/scanner.cc#newc... src/parsing/scanner.cc:1108: octal_pos_ = Location(start_pos, source_pos()); So, if we want to have a different error message for this, we could save it as `decimal_leading_zero_pos_` or something, and pick which one to report in CheckOctalLiteral()
As noted at https://bugs.chromium.org/p/v8/issues/detail?id=4973 , I'm concerned about the web compatibility of this patch. Let's get some data before landing this error checking.
Description was changed from
==========
report error for NonOctalDecimalIntegerLiteral in strict mode
In short, if it's an error to write 012, it's also an error to write 018.
This code reuses the error detecting and reporting framework for octal
literals, and even reports the same message, which isn't quite accourate:
print(019);
^^^
SyntaxError: Octal literals are not allowed in strict mode.
In order to get a different error message, we'll need to add a field to
Scanner to either distinguish which kind of octal literal we encountered
last, or simply store the last known octal position in addition to the
last known decimal_that_looks_like_octal position.
BUG=v8:4973
==========
to
==========
report error for NonOctalDecimalIntegerLiteral in strict mode
In short, if it's an error to write 012, it's also an error to write 018.
This code reuses the error detecting and reporting framework for octal
literals, and even reports the same message, which isn't quite accourate:
print(019);
^^^
SyntaxError: Octal literals are not allowed in strict mode.
In order to get a different error message, we'll need to add a field to
Scanner to either distinguish which kind of octal literal we encountered
last, or simply store the last known octal position in addition to the
last known decimal_that_looks_like_octal position.
BUG=v8:4973
LOG=y
==========
According to v8.h, I still need to update V8Initializer.cpp in Chromium. I'm planning on getting a Chromium dev environment set up next week so I can address this.
Description was changed from
==========
report error for NonOctalDecimalIntegerLiteral in strict mode
In short, if it's an error to write 012, it's also an error to write 018.
This code reuses the error detecting and reporting framework for octal
literals, and even reports the same message, which isn't quite accourate:
print(019);
^^^
SyntaxError: Octal literals are not allowed in strict mode.
In order to get a different error message, we'll need to add a field to
Scanner to either distinguish which kind of octal literal we encountered
last, or simply store the last known octal position in addition to the
last known decimal_that_looks_like_octal position.
BUG=v8:4973
LOG=y
==========
to
==========
In parallel to the strict octal check that would reject `012` in strict mode,
this patch collects UseCounters for `089` in strict mode. The spec says this
should be an error, but this patch does not report it as such.
BUG=v8:4973
LOG=y
==========
lgtm https://codereview.chromium.org/1948403002/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1948403002/diff/60001/include/v8.h#newcode5615 include/v8.h:5615: // and V8PerIsolateData.cpp in Chromium. ...and histogram.xml (see https://codereview.chromium.org/1578143002 for a full example).
The CQ bit was checked by jwolfe@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/1948403002/#ps80001 (title: "don't forget histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948403002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by jwolfe@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/1948403002/#ps100001 (title: "fix uninitialized field. guard against null")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948403002/100001
Message was sent while issue was closed.
Description was changed from ========== In parallel to the strict octal check that would reject `012` in strict mode, this patch collects UseCounters for `089` in strict mode. The spec says this should be an error, but this patch does not report it as such. BUG=v8:4973 LOG=y ========== to ========== In parallel to the strict octal check that would reject `012` in strict mode, this patch collects UseCounters for `089` in strict mode. The spec says this should be an error, but this patch does not report it as such. BUG=v8:4973 LOG=y ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== In parallel to the strict octal check that would reject `012` in strict mode, this patch collects UseCounters for `089` in strict mode. The spec says this should be an error, but this patch does not report it as such. BUG=v8:4973 LOG=y ========== to ========== In parallel to the strict octal check that would reject `012` in strict mode, this patch collects UseCounters for `089` in strict mode. The spec says this should be an error, but this patch does not report it as such. BUG=v8:4973 LOG=y Committed: https://crrev.com/d0b6686c14339bd5d0aeaf610705c7ed85393e1f Cr-Commit-Position: refs/heads/master@{#36221} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d0b6686c14339bd5d0aeaf610705c7ed85393e1f Cr-Commit-Position: refs/heads/master@{#36221}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1970333004/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20debug%20....
Message was sent while issue was closed.
https://codereview.chromium.org/1948403002/diff/100001/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1948403002/diff/100001/src/parsing/scanner.cc... src/parsing/scanner.cc:990: int start_pos; // For reporting octal positions. Maybe rename this to `octal_start_pos` and initialize it here --- though I think you should be just fine to initialize this within the `c0_ == '0'` branch (provided you don't use it outside of the branch). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
