[parser]: Skipping inner funcs / initial implemetation of storing scope analysis data from preparse…

[parser]: Skipping inner funcs / initial implemetation of storing scope analysis data from preparsed scopes.

The data produced at the moment only contains information about scope type +
positions, and only the most trivial tests pass.

Upcoming CLs will extend the data to contain information about variables (once
PreParser can produce it) and add more test cases.

BUG=v8:5516
Review-Url: https://codereview.chromium.org/2650703003
Cr-Commit-Position: refs/heads/master@{#42656}
Committed: https://chromium.googlesource.com/v8/v8/+/6053f4a331ac7700cd63eb35e3ba2afb0b51930f

[marja, 2017-01-24 10:48:24]

Description was changed from

==========
[parser]: Skipping inner funcs / initial implemetation of storing scope analysis
data from preparsed scopes.

The data produced is at the moment empty, and the test doesn't yet test
anything, since the PreParser cannot produce correct scope data for
anything non-trivial.

BUG=v8:5516
==========

to

==========
[parser]: Skipping inner funcs / initial implemetation of storing scope analysis
data from preparsed scopes.

The data produced at the moment only contains information about scope type +
positions, and only the most trivial tests pass.

Upcoming CLs will extend the data to contain information about variables (once
PreParser can produce it) and add more test cases.

BUG=v8:5516
==========

[marja, 2017-01-24 10:48:24]

marja@chromium.org changed reviewers:
+ neis@chromium.org

[marja, 2017-01-24 10:50:13]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 10:50:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2017-01-24 10:50:24]

neis, ptal

verwaest, adamk, fyi

[commit-bot: I haz the power, 2017-01-24 10:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 10:56:13]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[marja, 2017-01-24 12:30:21]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 12:30:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2017-01-25 09:20:28]

marja@chromium.org changed reviewers:
+ vogelheim@chromium.org
- neis@chromium.org

[marja, 2017-01-25 09:20:28]

ptal

[vogelheim, 2017-01-25 10:00:18]

lgtm

(Just for my understanding: This currently only collects data, and doesn't yet
do anything with it, right?)

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1382: PreParsedScopeData* preparsed_scope_data) {
DCHECK_EQ(FLAG_preparser_scope_analysis, preparsed_scope_data != nullptr);

(Or is this not the right invariant?)

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:2163: void Scope::GetAllocationData(PreParsedScopeData* data)
{
naming nitpick: This doesn't really get anything...  Maybe
CollectAllocationDataFromInnerScopes... or something shorter? :)

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:2169: }
nitpick: I found this code a bit weird to read... this iterates over the tree of
inner scopes (relative to the calling DeclarationScope::AnalyzePartially), but
it doesn't seem to actually do anything. Until I remembered that the ScopeData
constructor effectively builds up a chain of scope data referenced from within
*data.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/parser.h#ne...
src/parsing/parser.h:1140: PreParsedScopeData* preparsed_scope_data_;
nitpick: forward-declare PreParsedScopeData?
(It's included indirectly via parse-info, I guess.)

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
File src/parsing/preparsed-scope-data.h (right):

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:19: class ScopeData {
maybe: Have a PreParsedScopeData::AddScope(ScopeType, int, int) method.

Then the ScopeData structure can be a non-public implementation detail of
PreParsedScopeData, and we need one less friend declaration.

If the destructor is important, that method could return an (otherwise opaque)
scope object that calls a close method of sorts.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:19: class ScopeData {
troll nitpick: V8 style calls objects that open+finish some work in their
con-/de-structors **Scope. Please rename to ScopeDataScope.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:26: // will appar here.
nitpick: appar -> appear.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:53: #endif  // PREPARSED_SCOPE_DATA_H_
nitpick: // V8_PARSING_PREPARSED_SCOPE_DATA_H_

[vogelheim, 2017-01-25 10:00:22]

lgtm

(Just for my understanding: This currently only collects data, and doesn't yet
do anything with it, right?)

[marja, 2017-01-25 10:21:26]

Thanks for review!

Yes, this currently just collects scope positions, and checks it in the test.

Next steps (big picture):
- Add data about variables (for that I need to enhance the preparser a lot since
it just doesn't have the data yet) - tests will reflect what's the current
status of that enhancement.
- Actually use the data for skipping inner functions.
- Better data format.

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1382: PreParsedScopeData* preparsed_scope_data) {
On 2017/01/25 10:00:17, vogelheim wrote:
> DCHECK_EQ(FLAG_preparser_scope_analysis, preparsed_scope_data != nullptr);
> 
> (Or is this not the right invariant?)

It's not, since PreParsedScopeData always exists (it's a member of ParseInfo)
even without the flag. I could also not thread it through here unless the flag
is on (so I'd check the flag in Parser ctor or in ParseFunctionLiteral) but that
seems an unnecessary check.

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:2163: void Scope::GetAllocationData(PreParsedScopeData* data)
{
On 2017/01/25 10:00:18, vogelheim wrote:
> naming nitpick: This doesn't really get anything...  Maybe
> CollectAllocationDataFromInnerScopes... or something shorter? :)

Went for CollectVariableData (no "FromInenrScopes" since it also collects from
this scope).

https://codereview.chromium.org/2650703003/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:2169: }
On 2017/01/25 10:00:18, vogelheim wrote:
> nitpick: I found this code a bit weird to read... this iterates over the tree
of
> inner scopes (relative to the calling DeclarationScope::AnalyzePartially), but
> it doesn't seem to actually do anything. Until I remembered that the ScopeData
> constructor effectively builds up a chain of scope data referenced from within
> *data.

Acknowledged.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/parser.h#ne...
src/parsing/parser.h:1140: PreParsedScopeData* preparsed_scope_data_;
On 2017/01/25 10:00:18, vogelheim wrote:
> nitpick: forward-declare PreParsedScopeData?
> (It's included indirectly via parse-info, I guess.)

Done.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
File src/parsing/preparsed-scope-data.h (right):

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:19: class ScopeData {
On 2017/01/25 10:00:18, vogelheim wrote:
> troll nitpick: V8 style calls objects that open+finish some work in their
> con-/de-structors **Scope. Please rename to ScopeDataScope.

Used this opportunity to rename it to ScopeScope which was the original name.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:19: class ScopeData {
On 2017/01/25 10:00:18, vogelheim wrote:
> maybe: Have a PreParsedScopeData::AddScope(ScopeType, int, int) method.
> 
> Then the ScopeData structure can be a non-public implementation detail of
> PreParsedScopeData, and we need one less friend declaration.
> 
> If the destructor is important, that method could return an (otherwise opaque)
> scope object that calls a close method of sorts.

Acknowledged.

Offline discussion: Won't do anything for now; let's see how the next stage
looks like. Variable adding functions will be in ScopeScope.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:26: // will appar here.
On 2017/01/25 10:00:18, vogelheim wrote:
> nitpick: appar -> appear.

Done.

https://codereview.chromium.org/2650703003/diff/80001/src/parsing/preparsed-s...
src/parsing/preparsed-scope-data.h:53: #endif  // PREPARSED_SCOPE_DATA_H_
On 2017/01/25 10:00:18, vogelheim wrote:
> nitpick: // V8_PARSING_PREPARSED_SCOPE_DATA_H_

Done.

[marja, 2017-01-25 10:53:02]

The CQ bit was checked by marja@chromium.org

[marja, 2017-01-25 10:53:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2650703003/#ps100001
(title: "code review (vogelheim)")

[commit-bot: I haz the power, 2017-01-25 10:53:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 10:54:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 10:54:17]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19951)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16339)

[jochen (gone - plz use gerrit), 2017-01-25 11:16:00]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-25 11:16:01]

lgtm with nits

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
File src/parsing/preparsed-scope-data.cc (right):

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.cc:19: ++previous_scope_->inner_scope_count_;
inner_scope_count_ is just one-level down, right? It's not all inner scopes
count

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.cc:28: data->backing_store_.push_back(0);
maybe use -1, so it's easier to debug... right now, zero is a valid number for
number of vars (eg)

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
File src/parsing/preparsed-scope-data.h (right):

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:10: #include "src/globals.h"
for DISALLOW_COPY_AND_ASSIGN, you only need src/base/macros.h

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:17: PreParsedScopeData() :
current_scope_(nullptr) {}
nit. don't inline non-trivial ctors, and add a dtor

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:39: friend class ScopeScope;
nit, you don't need to friend inner classes

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:45: ScopeScope* current_scope_;
you can just declare this as ScopeScope* current_scope_ = nullptr;

[marja, 2017-01-25 11:50:29]

thanks for review

since the "parseinfo owns zone" stuff got reverted, i need to rebase this
again...

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
File src/parsing/preparsed-scope-data.cc (right):

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.cc:19: ++previous_scope_->inner_scope_count_;
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> inner_scope_count_ is just one-level down, right? It's not all inner scopes
> count

Yes. Why?

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.cc:28: data->backing_store_.push_back(0);
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> maybe use -1, so it's easier to debug... right now, zero is a valid number for
> number of vars (eg)

Done.

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
File src/parsing/preparsed-scope-data.h (right):

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:10: #include "src/globals.h"
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> for DISALLOW_COPY_AND_ASSIGN, you only need src/base/macros.h

For ScopeType I need globals.h.

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:17: PreParsedScopeData() :
current_scope_(nullptr) {}
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> nit. don't inline non-trivial ctors, and add a dtor

Confused about this comment (in the context of one below);  what's wrong with
inlining this? And now that I don't need to initialize current_scope_, should I
have an empty ctor which I'll define in the .cc file or what?

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:39: friend class ScopeScope;
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> nit, you don't need to friend inner classes

Done.

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.h:45: ScopeScope* current_scope_;
On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> you can just declare this as ScopeScope* current_scope_ = nullptr;

Done.

[jochen (gone - plz use gerrit), 2017-01-25 12:01:22]

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
File src/parsing/preparsed-scope-data.cc (right):

https://codereview.chromium.org/2650703003/diff/100001/src/parsing/preparsed-...
src/parsing/preparsed-scope-data.cc:19: ++previous_scope_->inner_scope_count_;
On 2017/01/25 at 11:50:28, marja wrote:
> On 2017/01/25 11:16:01, jochen (travelling til Feb 4) wrote:
> > inner_scope_count_ is just one-level down, right? It's not all inner scopes
> > count
> 
> Yes. Why?

just wanted to verify I understood the code correctly

[marja, 2017-01-25 12:23:01]

The CQ bit was checked by marja@chromium.org

[marja, 2017-01-25 12:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2650703003/#ps140001
(title: "rebased - argh, forced to format build.gn")

[commit-bot: I haz the power, 2017-01-25 12:23:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 13:03:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485346981872090,
"parent_rev": "f51a5f732c40af6c338956c0682d9dca92d1b34a", "commit_rev":
"6053f4a331ac7700cd63eb35e3ba2afb0b51930f"}

[commit-bot: I haz the power, 2017-01-25 13:03:24]

Description was changed from

==========
[parser]: Skipping inner funcs / initial implemetation of storing scope analysis
data from preparsed scopes.

The data produced at the moment only contains information about scope type +
positions, and only the most trivial tests pass.

Upcoming CLs will extend the data to contain information about variables (once
PreParser can produce it) and add more test cases.

BUG=v8:5516
==========

to

==========
[parser]: Skipping inner funcs / initial implemetation of storing scope analysis
data from preparsed scopes.

The data produced at the moment only contains information about scope type +
positions, and only the most trivial tests pass.

Upcoming CLs will extend the data to contain information about variables (once
PreParser can produce it) and add more test cases.

BUG=v8:5516

Review-Url: https://codereview.chromium.org/2650703003
Cr-Commit-Position: refs/heads/master@{#42656}
Committed:
https://chromium.googlesource.com/v8/v8/+/6053f4a331ac7700cd63eb35e3ba2afb0b5...
==========

[commit-bot: I haz the power, 2017-01-25 13:03:26]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/6053f4a331ac7700cd63eb35e3ba2afb0b5...