Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599253002:
[counters] Add UseCounters for 'f() = 0' syntax (Closed)
Description[counters] Add UseCounters for 'f() = 0' syntax
This syntax was formerly legal per ECMAScript, but has been a
SyntaxError for some time now. V8 deviates from spec in that it
is instead a runtime error; we'd like to know if we can get
away with removing it (at least in sloppy mode) or if the spec
should be changed.
c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880
Also add self to authors file
BUG=v8:4480
Review-Url: https://codereview.chromium.org/2599253002
Cr-Commit-Position: refs/heads/master@{#41960}
Committed: https://chromium.googlesource.com/v8/v8/+/bf9e013bbc52f8fb3040a15fb71c8abfc0c15de7
Patch Set 1 #
Total comments: 5
Patch Set 2 : address dan's comments #Patch Set 3 : fix test #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. Also add self to authors file BUG= ========== to ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880 Also add self to authors file BUG= ==========
bakkot@gmail.com changed reviewers: + adamk@chromium.org, littledan@chromium.org
PTAL
Description was changed from ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880 Also add self to authors file BUG= ========== to ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880 Also add self to authors file BUG=v8:4480 ==========
Awesome! Keep doing this! Just a couple nits. I added a reference to the relevant bug in the commit message. https://codereview.chromium.org/2599253002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2599253002/diff/1/include/v8.h#newcode6491 include/v8.h:6491: kAssigmentExpressionLHSIsCallInNonstrict = 36, Bikeshed: We use 'sloppy' all over the place to express non-strict; I think it'd be clearer to use that here too. https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h#o... src/parsing/parser-base.h:4334: // Rewrite `expr' to `expr[throw ReferenceError]'. Could you put a reference to https://bugs.chromium.org/p/v8/issues/detail?id=4480 here? https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:4336: is_strict(language_mode()) Aren't you glad we don't have strong mode?
https://codereview.chromium.org/2599253002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2599253002/diff/1/include/v8.h#newcode6491 include/v8.h:6491: kAssigmentExpressionLHSIsCallInNonstrict = 36, On 2016/12/23 07:56:52, Dan Ehrenberg wrote: > Bikeshed: We use 'sloppy' all over the place to express non-strict; I think it'd > be clearer to use that here too. Done. https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2599253002/diff/1/src/parsing/parser-base.h#o... src/parsing/parser-base.h:4334: // Rewrite `expr' to `expr[throw ReferenceError]'. On 2016/12/23 07:56:52, Dan Ehrenberg wrote: > Could you put a reference to > https://bugs.chromium.org/p/v8/issues/detail?id=4480 here? Done.
lgtm
The CQ bit was checked by bakkot@gmail.com 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: 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...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
On 2016/12/23 12:27:21, Dan Ehrenberg wrote: > lgtm The UseCounter is triggered once for each parse (which I should have realized), which means a given usage gets double-counted if it's in code which gets optimized. There are two existing UseCounters (kDecimalWithLeadingZeroInStrictMode and kLegacyFunctionDeclaration) which I think would suffer from the same issue, except the first of them is only triggered on an error and the second is afaict unreachable. Not sure what to do about this. Seeing as we're only interested in rough numbers, maybe it's ok to just double-count sometimes?
On 2016/12/23 20:51:47, bakkot1 wrote: > On 2016/12/23 12:27:21, Dan Ehrenberg wrote: > > lgtm > > The UseCounter is triggered once for each parse (which I should have realized), > which means a given usage gets double-counted if it's in code which gets > optimized. There are two existing UseCounters > (kDecimalWithLeadingZeroInStrictMode and kLegacyFunctionDeclaration) which I > think would suffer from the same issue, except the first of them is only > triggered on an error and the second is afaict unreachable. > > Not sure what to do about this. Seeing as we're only interested in rough > numbers, maybe it's ok to just double-count sometimes? All that matters for UseCounters is triggered or not triggered for a page load. So, no, double counting doesn't do anything.
The CQ bit was checked by bakkot@gmail.com 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.
The CQ bit was checked by bakkot@gmail.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/2599253002/#ps40001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482859055942790,
"parent_rev": "e0359c36294e7fda024d075eca40c3b773f5abb9", "commit_rev":
"bf9e013bbc52f8fb3040a15fb71c8abfc0c15de7"}
Message was sent while issue was closed.
Description was changed from ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880 Also add self to authors file BUG=v8:4480 ========== to ========== [counters] Add UseCounters for 'f() = 0' syntax This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. https://github.com/tc39/ecma262/issues/257#issuecomment-195106880 Also add self to authors file BUG=v8:4480 Review-Url: https://codereview.chromium.org/2599253002 Cr-Commit-Position: refs/heads/master@{#41960} Committed: https://chromium.googlesource.com/v8/v8/+/bf9e013bbc52f8fb3040a15fb71c8abfc0c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/bf9e013bbc52f8fb3040a15fb71c8abfc0c...
Message was sent while issue was closed.
Alright, this has landed. Dan, could you take care of // If you add new values here, you'll also need to update Chromium's: // UseCounter.h, V8PerIsolateData.cpp, histograms.xml 6495 // UseCounter.h, V8PerIsolateData.cpp, histograms.xml ? Might need to wait until Chromium rolls v8; I forget. (I would, but I don't have an active Chromium checkout, and it's a right pain to acquire and build locally.)
Message was sent while issue was closed.
jochen@chromium.org changed reviewers: + jochen@chromium.org
Message was sent while issue was closed.
any updates on the use counters?
Message was sent while issue was closed.
On 2017/01/11 12:44:11, jochen wrote: > any updates on the use counters? Oh sorry, I was going to lump in the Chrome side with some other use counters, but I keep not getting around to adding those other ones. I'll thread this up through Chrome within a week. Sorry for the delay.
Message was sent while issue was closed.
On 2017/01/11 12:44:11, jochen wrote: > any updates on the use counters? Oh sorry, I was going to lump in the Chrome side with some other use counters, but I keep not getting around to adding those other ones. I'll thread this up through Chrome within a week. Sorry for the delay.
Message was sent while issue was closed.
no sweat... I'll add the use counters together with another one I want to add anyway |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
