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

Issue 2683573002: [parser/test] Move cctest/PreParserScopeAnalysis into a new file. (Closed)

Created:
3 years, 10 months ago by marja
Modified:
3 years, 10 months ago
Reviewers:
neis, vogelheim
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[parser/test] Move cctest/PreParserScopeAnalysis into a new file. BUG=v8:5516 R=vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2683573002 Cr-Commit-Position: refs/heads/master@{#42986} Committed: https://chromium.googlesource.com/v8/v8/+/009e8b11e25c658b5990df54d146abe7ad11d666

Patch Set 1 #

Total comments: 4

Patch Set 2 : move a file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+491 lines, -449 lines) Patch
M test/cctest/BUILD.gn View 1 2 chunks +3 lines, -0 lines 0 comments Download
M test/cctest/cctest.gyp View 1 2 chunks +3 lines, -0 lines 0 comments Download
A test/cctest/parsing/test-preparser.cc View 1 1 chunk +363 lines, -0 lines 0 comments Download
A test/cctest/scope-test-helper.h View 1 chunk +88 lines, -0 lines 0 comments Download
M test/cctest/test-parsing.cc View 4 chunks +2 lines, -449 lines 0 comments Download
A test/cctest/unicode-helpers.h View 1 chunk +32 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 11 (4 generated)
marja
ptal
3 years, 10 months ago (2017-02-07 08:58:43 UTC) #1
vogelheim
https://codereview.chromium.org/2683573002/diff/1/test/cctest/test-preparser.cc File test/cctest/test-preparser.cc (right): https://codereview.chromium.org/2683573002/diff/1/test/cctest/test-preparser.cc#newcode1 test/cctest/test-preparser.cc:1: // Copyright 2017 the V8 project authors. All rights ...
3 years, 10 months ago (2017-02-07 09:11:23 UTC) #2
vogelheim
lgtm
3 years, 10 months ago (2017-02-07 09:16:17 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2683573002/20001
3 years, 10 months ago (2017-02-07 09:47:42 UTC) #6
neis
There are a few other preparser-only tests in test-parsing.cc, maybe you can move them as ...
3 years, 10 months ago (2017-02-07 09:59:37 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/009e8b11e25c658b5990df54d146abe7ad11d666
3 years, 10 months ago (2017-02-07 10:11:07 UTC) #10
marja
3 years, 10 months ago (2017-02-07 13:38:14 UTC) #11
Message was sent while issue was closed.
thanks for review

I intentionally didn't move other tests; trying to not spend too much energy on
test reorganization and simultaneously be a good... or at least a non-horrible
citizen.

https://codereview.chromium.org/2683573002/diff/1/test/cctest/test-preparser.cc
File test/cctest/test-preparser.cc (right):

https://codereview.chromium.org/2683573002/diff/1/test/cctest/test-preparser....
test/cctest/test-preparser.cc:1: // Copyright 2017 the V8 project authors. All
rights reserved.
On 2017/02/07 09:11:23, vogelheim wrote:
> I'd be slightly happier if this file were in the parsing/ subdir, i.e.,
> test/cctest/parsing/test-preparser.cc.

Done.

https://codereview.chromium.org/2683573002/diff/1/test/cctest/test-preparser....
test/cctest/test-preparser.cc:26: int suffix_len = Utf8LengthHelper(suffix);
On 2017/02/07 09:11:23, vogelheim wrote:
> nitpick, and entirely unrelated to this CL: With the given prefix/suffix,
strlen
> would compute the length just fine.

Lol, this was some brainless copy paste...

However, kept this as is. Probably makes sense to move the utf helpers anyway.

Powered by Google App Engine
This is Rietveld 408576698