Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(20)

Issue 2009963002: [modules] Disable HTML-like comments

Created:
3 years, 3 months ago by mike3
Modified:
2 years, 11 months ago
Reviewers:
adamk, vogelheim
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.

Description

[modules] Disable HTML-like comments ES2015 section B.1.3 ("HTML-like Comments") reads: > The syntax and semantics of 11.4 is extended as follows except that > this extension is not allowed when parsing source code using the goal > symbol Module http://www.ecma-international.org/ecma-262/6.0/#sec-html-like-comments BUG=v8:5045 LOG=N R=adamk@chromium.org

Patch Set 1 #

Total comments: 2

Patch Set 2 : Update attribute/method name #

Total comments: 6

Patch Set 3 : Refactor #

Patch Set 4 : Unify behavior for optimized and non-optimized executions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -99 lines) Patch
M src/ast/scopes.h View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M src/ast/scopes.cc View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M src/objects.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 chunks +5 lines, -4 lines 0 comments Download
M src/parsing/scanner.h View 1 2 2 chunks +6 lines, -1 line 0 comments Download
M src/parsing/scanner.cc View 1 2 6 chunks +9 lines, -5 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 14 chunks +98 lines, -89 lines 0 comments Download
A test/mjsunit/harmony/modules-html-comments.js View 1 2 3 1 chunk +46 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (2 generated)
mike3
3 years, 3 months ago (2016-05-25 20:38:44 UTC) #1
mike3
Hi Adam, This patch does not pass mjsunit. The test I added fails a CHECK: ...
3 years, 3 months ago (2016-05-25 20:41:21 UTC) #3
adamk
+vogelheim who recently made a similar change for other reasons. His patch (since reverted), only ...
3 years, 3 months ago (2016-05-25 20:43:02 UTC) #5
mike3
https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc#newcode45 src/parsing/scanner.cc:45: module_(false), On 2016/05/25 20:43:02, adamk wrote: > Please call ...
3 years, 3 months ago (2016-05-25 21:10:14 UTC) #6
vogelheim
On 2016/05/25 20:43:02, adamk wrote: > +vogelheim who recently made a similar change for other ...
3 years, 3 months ago (2016-05-27 08:07:10 UTC) #7
vogelheim
I suspect this is broken. Please be sure that 1, Parser + Pre-Parser still work ...
3 years, 3 months ago (2016-05-27 08:08:49 UTC) #8
mike3
Thanks for the review! I've researched how the scanner is used a bit more, and ...
3 years, 3 months ago (2016-05-27 15:53:22 UTC) #9
mike3
I've extended JSFunction to carry a flag for whether it originated from module code or ...
3 years, 3 months ago (2016-05-30 21:57:00 UTC) #10
mike3
> Hi Mike, > > ... talked to Yang; we have a plan now: > ...
3 years, 3 months ago (2016-06-06 22:08:33 UTC) #11
adamk
Hi Mike, Did you still want to give a shot at fixing this bug? I ...
2 years, 11 months ago (2016-10-12 20:41:03 UTC) #12
mike3
2 years, 11 months ago (2016-10-16 16:07:29 UTC) #13
Hey Adam,

Sure, I'd love to follow through on this patch--thanks for checking back with
me! I've seen a lot of activity in service of bug 1569, and I was starting to
worry that my work for "track module code" [1] would be obsoleted. It sounds
like that's the case, but I understand how these things go. Should I close that
ticket?

In either case, I'm all ears for any input you have here!

[1] https://codereview.chromium.org/2065453002/

On 2016/10/12 20:41:03, adamk wrote:
> Hi Mike,
> 
> Did you still want to give a shot at fixing this bug? I think it may be
simpler
> than we originally thought (due to some facts about how modules are compiled).
> Let me know if you're still interested, and we can chat about how to simplify
> the approach (for one thing, I'm not sure we actually need your "track module
> code" patch).

Powered by Google App Engine
This is Rietveld 408576698