Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(110)

Issue 663683006: Implement ES6 Template Literals (Closed)

Created:
6 years, 1 month ago by caitp (gmail)
Modified:
6 years ago
CC:
v8-dev, adamk
Project:
v8
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 4

Patch Set 2 : Remove Scanner::Mode, among other things #

Patch Set 3 : Make untagged template literals actually work #

Patch Set 4 : Implement tagged template literals #

Patch Set 5 : Add test cases for harmony-templates #

Total comments: 2

Patch Set 6 : Fix scanner bugs, add more tests #

Patch Set 7 : Prevent fall-through to template token handlers #

Total comments: 1

Patch Set 8 : Remove TemplateLiteral AST node, do it all in parsing #

Total comments: 9

Patch Set 9 : some extra parsing tests #

Patch Set 10 : More tests again #

Total comments: 10

Patch Set 11 : Add comments describing the parsing and scanning of TemplateLiterals #

Total comments: 1

Patch Set 12 : Tiny fixups #

Total comments: 9

Patch Set 13 : More comments + nits #

Patch Set 14 : Don't record raw literals #

Patch Set 15 : Remove kTemplateLiteral bailout-reason #

Total comments: 9

Patch Set 16 : Additional TRV tests #

Patch Set 17 : Fix issue with line-continuation, nits #

Total comments: 15

Patch Set 18 : Rename temporary variables for readability #

Total comments: 1

Patch Set 19 : ensure scanner flag is initialized to false #

Total comments: 2

Patch Set 20 : Reorganize generation of raw template strings, slightly #

Patch Set 21 : Report unterminated template literals error #

Patch Set 22 : Simplify GetTemplateCallSite() #

Total comments: 5

Patch Set 23 : Add more redundant and thorough mjsunit tests #

Total comments: 6

Patch Set 24 : Better callSiteObj shape-testing, move tests to harmony/templates.js #

Total comments: 1

Patch Set 25 : Better error reporting + use Array.isArray() rather than instanceof #

Patch Set 26 : Fix trailing space #

Patch Set 27 : Even better static error reporting #

Total comments: 9

Patch Set 28 : Nits + extra parsing tests (pre clang-format) #

Patch Set 29 : Clang-formatted #

Total comments: 1

Patch Set 30 : Add harmony-templates to BUILD.gn #

Patch Set 31 : Get rid of nested parentheses #

Patch Set 32 : Fix failing DCHECKs in parser.cc and preparser.h #

Patch Set 33 : Rebased against https://chromium.googlesource.com/v8/v8.git/master #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+906 lines, -8 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -1 line 0 comments Download
M src/ast-value-factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1 line, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +4 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments Download
A src/harmony-templates.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +14 lines, -0 lines 0 comments Download
M src/messages.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +3 lines, -0 lines 0 comments Download
M src/parser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +69 lines, -0 lines 0 comments Download
M src/parser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +139 lines, -0 lines 0 comments Download
M src/preparser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 9 chunks +126 lines, -3 lines 3 comments Download
M src/scanner.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +7 lines, -0 lines 0 comments Download
M src/scanner.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +79 lines, -1 line 0 comments Download
M src/token.h View 1 chunk +5 lines, -1 line 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 4 chunks +113 lines, -1 line 0 comments Download
A test/mjsunit/harmony/templates.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +341 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 89 (11 generated)
caitp (gmail)
Thought I'd have a go at it. It's not done, but parsing sort of works ...
6 years, 1 month ago (2014-10-27 17:27:39 UTC) #1
arv (Not doing code reviews)
Neat. https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h File src/bailout-reason.h (right): https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h#newcode326 src/bailout-reason.h:326: V(kTemplateLiteral, "Template Literal") Keep in alphabetical order. https://codereview.chromium.org/663683006/diff/1/src/scanner.h ...
6 years, 1 month ago (2014-10-27 18:14:06 UTC) #3
caitp (gmail)
https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h File src/bailout-reason.h (right): https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h#newcode326 src/bailout-reason.h:326: V(kTemplateLiteral, "Template Literal") On 2014/10/27 18:14:06, arv wrote: > ...
6 years, 1 month ago (2014-10-27 20:22:36 UTC) #4
Dmitry Lomov (no reviews)
Before this CL can land, I think we need a clear design for implementation of ...
6 years, 1 month ago (2014-10-28 14:25:14 UTC) #6
caitp (gmail)
On 2014/10/28 14:25:14, Dmitry Lomov (chromium) wrote: > Before this CL can land, I think ...
6 years, 1 month ago (2014-10-28 20:26:07 UTC) #7
caitp (gmail)
I've implemented arv@'s suggestion to generate code for untagged templates as a tree of BinaryOps ...
6 years, 1 month ago (2014-10-29 01:06:56 UTC) #8
Dmitry Lomov (no reviews)
On 2014/10/29 01:06:56, caitp wrote: > I've implemented arv@'s suggestion to generate code for untagged ...
6 years, 1 month ago (2014-10-29 11:07:14 UTC) #10
caitp (gmail)
I know I already sent an invite to reviewers, but posting it here so others ...
6 years, 1 month ago (2014-10-29 17:17:32 UTC) #11
caitp (gmail)
Added rudimentary (slightly incorrect) support for tagged templates. This doesn't include any caching strategy, so ...
6 years, 1 month ago (2014-11-01 02:22:22 UTC) #12
caitp (gmail)
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js#newcode76 test/mjsunit/es6/templates.js:76: // assertEquals("", s.raw[0]); If I could get some help ...
6 years, 1 month ago (2014-11-01 03:33:56 UTC) #14
caitp (gmail)
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js#newcode76 test/mjsunit/es6/templates.js:76: // assertEquals("", s.raw[0]); On 2014/11/01 03:33:55, caitp wrote: > ...
6 years, 1 month ago (2014-11-01 04:34:21 UTC) #15
caitp (gmail)
Just FWIW, https://www.dropbox.com/s/5f9sq0tqifkmsvc/spidermonkey_does_it_wrong_too.png?dl=0 SpiderMonkey is not caching the CallSite either, so maybe it's not a ...
6 years, 1 month ago (2014-11-01 06:17:58 UTC) #16
caitp (gmail)
On 2014/11/01 06:17:58, caitp wrote: > Just FWIW, > https://www.dropbox.com/s/5f9sq0tqifkmsvc/spidermonkey_does_it_wrong_too.png?dl=0 > SpiderMonkey is not caching ...
6 years, 1 month ago (2014-11-01 11:23:02 UTC) #17
caitp (gmail)
https://codereview.chromium.org/663683006/diff/160001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/160001/src/parser.cc#newcode5029 src/parser.cc:5029: So when it comes to caching the callSiteObj, something ...
6 years, 1 month ago (2014-11-03 20:30:09 UTC) #18
rossberg
Given that the spec resolution of the caching semantics will probably take a while (I ...
6 years, 1 month ago (2014-11-07 13:03:28 UTC) #19
caitp (gmail)
On 2014/11/07 13:03:28, rossberg wrote: > Given that the spec resolution of the caching semantics ...
6 years, 1 month ago (2014-11-07 13:45:36 UTC) #20
rossberg
On 2014/11/07 13:45:36, caitp wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > Given that ...
6 years, 1 month ago (2014-11-07 14:00:47 UTC) #21
caitp (gmail)
On 2014/11/07 13:45:36, caitp wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > Given that ...
6 years, 1 month ago (2014-11-07 14:01:14 UTC) #22
caitp (gmail)
On 2014/11/07 14:00:47, rossberg wrote: > On 2014/11/07 13:45:36, caitp wrote: > > On 2014/11/07 ...
6 years, 1 month ago (2014-11-07 14:02:32 UTC) #23
arv (Not doing code reviews)
On 2014/11/07 14:00:47, rossberg wrote: > ... (The caching > is mostly a performance hack ...
6 years, 1 month ago (2014-11-07 14:18:36 UTC) #24
rossberg
On 2014/11/07 14:18:36, arv wrote: > On 2014/11/07 14:00:47, rossberg wrote: > > ... (The ...
6 years, 1 month ago (2014-11-07 14:32:55 UTC) #25
Dmitry Lomov (no reviews)
On 2014/11/07 13:03:28, rossberg wrote: > Given that the spec resolution of the caching semantics ...
6 years, 1 month ago (2014-11-07 15:22:59 UTC) #26
caitp (gmail)
On 2014/11/07 15:22:59, Dmitry Lomov (chromium) wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > ...
6 years, 1 month ago (2014-11-07 15:30:02 UTC) #27
rossberg
On 2014/11/07 15:30:02, caitp wrote: > On 2014/11/07 15:22:59, Dmitry Lomov (chromium) wrote: > > ...
6 years, 1 month ago (2014-11-07 15:39:17 UTC) #28
Dmitry Lomov (no reviews)
On 2014/11/07 15:39:17, rossberg wrote: > On 2014/11/07 15:30:02, caitp wrote: > > On 2014/11/07 ...
6 years, 1 month ago (2014-11-07 16:38:40 UTC) #29
arv (Not doing code reviews)
Adding marja since she is the parser expert. One thing that I was thinking is ...
6 years, 1 month ago (2014-11-07 16:55:50 UTC) #31
caitp (gmail)
I agree it would be nice to get the raw text in a better way, ...
6 years, 1 month ago (2014-11-07 17:13:08 UTC) #32
arv (Not doing code reviews)
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js#newcode28 test/mjsunit/es6/templates.js:28: // Multi-line templates On 2014/11/07 17:13:08, caitp wrote: > ...
6 years, 1 month ago (2014-11-07 17:14:18 UTC) #33
caitp (gmail)
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js#newcode28 test/mjsunit/es6/templates.js:28: // Multi-line templates On 2014/11/07 17:14:18, arv wrote: > ...
6 years, 1 month ago (2014-11-07 17:21:00 UTC) #34
marja
I started having a look, but then at some point my confusion exceeded a threshold. ...
6 years, 1 month ago (2014-11-10 15:32:02 UTC) #35
caitp (gmail)
Thanks for the look --- I will add some comments to explain things a bit, ...
6 years, 1 month ago (2014-11-10 15:43:56 UTC) #36
caitp (gmail)
Added some comments to explain the parsing and scanning of template literals, I hope that ...
6 years, 1 month ago (2014-11-10 16:07:39 UTC) #37
marja
Thx; it's much clearer now. I didn't yet have a close look at CloseTemplateLiteral, will ...
6 years, 1 month ago (2014-11-10 16:18:29 UTC) #38
marja
caitp, wdyt about arv's idea: not storing the raw string but getting it from the ...
6 years, 1 month ago (2014-11-11 08:51:03 UTC) #39
marja
To continue our discussion in the old patch set... (To maximize confusion, I'll post new ...
6 years, 1 month ago (2014-11-11 09:15:36 UTC) #40
marja
Alright, here are some more comments. These are pretty much trivial. The biggest question mark ...
6 years, 1 month ago (2014-11-11 09:47:24 UTC) #41
caitp (gmail)
On 2014/11/11 08:51:03, marja wrote: > caitp, wdyt about arv's idea: not storing the raw ...
6 years, 1 month ago (2014-11-11 13:59:29 UTC) #42
marja
https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 src/scanner.cc:819: PushBack('}'); On 2014/11/11 13:59:29, caitp wrote: > On 2014/11/11 ...
6 years, 1 month ago (2014-11-11 15:01:56 UTC) #43
caitp (gmail)
On 2014/11/11 15:01:56, marja wrote: > https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc > File src/scanner.cc (right): > > https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 > ...
6 years, 1 month ago (2014-11-11 16:15:30 UTC) #44
marja
Alright, I'll have a look tomorrow since the day is over on this time zone. ...
6 years, 1 month ago (2014-11-11 16:25:14 UTC) #45
arv (Not doing code reviews)
Very nice. https://codereview.chromium.org/663683006/diff/320001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/320001/src/harmony-templates.js#newcode14 src/harmony-templates.js:14: for (; index < count; ++index) { ...
6 years, 1 month ago (2014-11-11 17:15:15 UTC) #46
caitp (gmail)
https://codereview.chromium.org/663683006/diff/320001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/320001/src/parser.cc#newcode5138 src/parser.cc:5138: if (fni_ != NULL) fni_->RemoveLastFunction(); On 2014/11/11 17:15:15, arv ...
6 years, 1 month ago (2014-11-11 17:48:40 UTC) #47
caitp (gmail)
6 years, 1 month ago (2014-11-11 17:48:45 UTC) #48
marja
LG modulo these comments. There's one bug (missing setting the flag), and the rest is ...
6 years, 1 month ago (2014-11-12 09:23:22 UTC) #51
marja
Oh and pls change the description of the CL to something more descriptive as a ...
6 years, 1 month ago (2014-11-12 09:24:02 UTC) #52
marja
Can somebody (arv@?) have a look at src/harmony-templates.js, I can't say whether it's correct or ...
6 years, 1 month ago (2014-11-12 10:16:53 UTC) #53
caitp (gmail)
Addressed some nits... Last thing is probably to move generation of "raw" strings into own ...
6 years, 1 month ago (2014-11-12 13:33:12 UTC) #54
marja
LGTM modulo the uninitialized variable below. pls wait for review for src/harmony-templates.js before committing. https://codereview.chromium.org/663683006/diff/420001/src/scanner.h ...
6 years, 1 month ago (2014-11-12 13:39:32 UTC) #55
marja
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) ...
6 years, 1 month ago (2014-11-12 13:41:14 UTC) #56
caitp (gmail)
On 2014/11/12 13:41:14, marja wrote: > https://codereview.chromium.org/663683006/diff/400001/src/parser.cc > File src/parser.cc (right): > > https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 > ...
6 years, 1 month ago (2014-11-12 14:34:59 UTC) #57
arv (Not doing code reviews)
https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js#newcode16 src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop, ToPropertyDescriptor({ I feel like this is doing ...
6 years, 1 month ago (2014-11-12 15:06:05 UTC) #58
caitp (gmail)
https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js#newcode16 src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop, ToPropertyDescriptor({ On 2014/11/12 15:06:05, arv wrote: > ...
6 years, 1 month ago (2014-11-12 15:12:32 UTC) #59
arv (Not doing code reviews)
A few more tests would be useful to prevent regressions. https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templates.js#newcode38 ...
6 years, 1 month ago (2014-11-12 15:43:15 UTC) #60
caitp (gmail)
https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templates.js#newcode38 test/mjsunit/es6/templates.js:38: (function testLineCondition() { On 2014/11/12 15:43:15, arv wrote: > ...
6 years, 1 month ago (2014-11-12 16:12:18 UTC) #61
arv (Not doing code reviews)
LGTM We keep the tests for unreleased feature in test/mjsunit/harmony/ and not in test/mjsunit/es6/. Once ...
6 years, 1 month ago (2014-11-12 16:19:11 UTC) #62
caitp (gmail)
https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templates.js File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templates.js#newcode134 test/mjsunit/es6/templates.js:134: (function(s) { calls++; assertEquals("foo", s[0]); })`foo`; On 2014/11/12 16:19:10, ...
6 years, 1 month ago (2014-11-12 16:37:05 UTC) #63
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/templates.js File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/templates.js#newcode297 test/mjsunit/harmony/templates.js:297: assertTrue(cs.raw instanceof Array); Similar issue with instanceof as ...
6 years, 1 month ago (2014-11-12 17:03:38 UTC) #64
marja
caitp, should we land this now?
6 years, 1 month ago (2014-11-13 10:48:20 UTC) #65
caitp (gmail)
On 2014/11/13 10:48:20, marja wrote: > caitp, should we land this now? I made some ...
6 years, 1 month ago (2014-11-13 14:57:05 UTC) #66
marja
On 2014/11/13 14:57:05, caitp wrote: > On 2014/11/13 10:48:20, marja wrote: > > caitp, should ...
6 years, 1 month ago (2014-11-13 15:04:03 UTC) #67
caitp (gmail)
On 2014/11/13 15:04:03, marja wrote: > Btw, I would've wanted to have a look at ...
6 years, 1 month ago (2014-11-13 15:15:29 UTC) #68
arv (Not doing code reviews)
I was hoping Dimitri or Andreas would review this too.
6 years, 1 month ago (2014-11-13 16:49:02 UTC) #69
arv (Not doing code reviews)
On 2014/11/13 16:49:02, arv wrote: > I was hoping Dimitri or Andreas would review this ...
6 years, 1 month ago (2014-11-13 16:49:30 UTC) #70
rossberg
Looks good, just a few nits and some more tests. https://codereview.chromium.org/663683006/diff/600001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5054 ...
6 years, 1 month ago (2014-11-14 12:28:07 UTC) #71
caitp (gmail)
Thanks for the look, I've re-arranged code a bit to address the style issues, and ...
6 years, 1 month ago (2014-11-14 13:25:18 UTC) #72
rossberg
lgtm https://codereview.chromium.org/663683006/diff/640001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/640001/src/parser.cc#newcode5152 src/parser.cc:5152: if ((from_index + 1) < length && raw_chars[from_index ...
6 years, 1 month ago (2014-11-14 13:38:30 UTC) #73
arv (Not doing code reviews)
Can you also update the BUILD.gn file?
6 years, 1 month ago (2014-11-14 14:40:02 UTC) #74
caitp (gmail)
On 2014/11/14 14:40:02, arv wrote: > Can you also update the BUILD.gn file? Done (I ...
6 years, 1 month ago (2014-11-14 14:59:04 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663683006/670001
6 years, 1 month ago (2014-11-14 16:00:18 UTC) #77
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/28) v8_linux64_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/1237) v8_linux_rel ...
6 years, 1 month ago (2014-11-14 16:01:40 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663683006/710001
6 years, 1 month ago (2014-11-14 18:25:34 UTC) #81
commit-bot: I haz the power
Committed patchset #33 (id:710001)
6 years, 1 month ago (2014-11-14 18:53:54 UTC) #82
brucedawson
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode2816 src/preparser.h:2816: int pos = peek_position(); While running VC++'s /analyze on ...
6 years ago (2014-12-09 22:37:13 UTC) #84
Dmitry Lomov (no reviews)
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode2816 src/preparser.h:2816: int pos = peek_position(); On 2014/12/09 22:37:13, brucedawson wrote: ...
6 years ago (2014-12-09 22:40:49 UTC) #85
Dmitry Lomov (no reviews)
6 years ago (2014-12-09 22:40:54 UTC) #86
caitp (gmail)
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode2816 src/preparser.h:2816: int pos = peek_position(); On 2014/12/09 22:40:49, Dmitry Lomov ...
6 years ago (2014-12-09 22:42:15 UTC) #87
brucedawson
Renaming it would be nice, depending on the tradeoff you want between code clarity and ...
6 years ago (2014-12-09 22:49:03 UTC) #88
Dmitry Lomov (no reviews)
6 years ago (2014-12-09 22:53:20 UTC) #89
Message was sent while issue was closed.
On 2014/12/09 22:42:15, caitp wrote:
> https://codereview.chromium.org/663683006/diff/710001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode...
> src/preparser.h:2816: int pos = peek_position();
> On 2014/12/09 22:40:49, Dmitry Lomov (chromium) wrote:
> > On 2014/12/09 22:37:13, brucedawson wrote:
> > > While running VC++'s /analyze on Chrome I got a warning for this line of
> code,
> > > pointing out that the definition of 'pos' shadows another definition in
the
> > > outer scope. These warnings are fairly common, but this one looks
> potentially
> > > wrong. The use of 'pos' in the ReportMessageAt call a few lines down looks
> > like
> > > it might be intended to use the outer-scope pos. At the very least the
> intent
> > is
> > > not clear.
> > > 
> > > Any thoughts?
> > 
> > The usage looks good (we are reporting the error from pos to peek_position()
> > which changes after we parse an expression.
> 
> We could rename it to avoid the warning, if that makes sense

Up to you Caitlin :) I am fine either way

Powered by Google App Engine
This is Rietveld 408576698