[interpreter] Minor clean-up of BytecodeSourceInfo.

[interpreter] Minor clean-up of BytecodeSourceInfo.

Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/303d340f669a8521708357432bfe2cb368d42b2f
Cr-Commit-Position: refs/heads/master@{#37136}

[oth, 2016-06-09 17:51:36]

Description was changed from

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.
    
Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N
==========

[oth, 2016-06-09 17:57:23]

oth@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[oth, 2016-06-09 17:57:24]

Ross, PTAL at this attempt to retire BytecodeSourceInfo::Update(), thanks.

[oth, 2016-06-10 13:54:07]

On 2016/06/09 17:57:24, oth wrote:
> Ross, PTAL at this attempt to retire BytecodeSourceInfo::Update(), thanks.

Ignore this request - there's a few test failures that need to be nixed before
reviewing this.

Thanks

[oth, 2016-06-13 05:29:55]

On 2016/06/10 13:54:07, oth wrote:
> On 2016/06/09 17:57:24, oth wrote:
> > Ross, PTAL at this attempt to retire BytecodeSourceInfo::Update(), thanks.
> 
> Ignore this request - there's a few test failures that need to be nixed before
> reviewing this.
> 
> Thanks

Test issues addressed, PTAL. Thanks!

[rmcilroy, 2016-06-20 11:21:39]

Nice refactor, thanks! Couple of comments and would like MakeInvalidEqualTo to
have a different name, but otherwise LGTM.

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:450: // is associated with these
positions, they are ignored for now.
/s/they are ignored for now/instead we force the stack check's expression
position which eliminates the empty statement's position/ (or something similar)

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:95: void MakeInvalidEqualTo(const
BytecodeSourceInfo& other) {
I'm not keen on this name.

One possible suggestion to avoid the need for this function - how about in
BytecodeArrayBuilder, instead of having a AttachSourceInfo, have a
GetSourcePositionInfo function which returns either the current source position
or a invalid one, and pass it directly to the BytecodeNode constructor in
BytecodeArrayBuilder::Output? I think the compiler should optimize away the
extra construct/copy. WDYT?

Just noticed you have other uses of this outside BytecodeArrayBuilder. I think
the above is still possibly worth while, however I guess you still need this
function either way. As such, could we rename it to Clone() or something
similar?

[rmcilroy, 2016-06-20 11:21:45]

Nice refactor, thanks! Couple of comments and would like MakeInvalidEqualTo to
have a different name, but otherwise LGTM.

[oth, 2016-06-21 10:05:46]

Thanks, comments incorporated.

I haven't gone with GetSourcePositionInfo. See the comment, and let me know.
Thanks.

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:450: // is associated with these
positions, they are ignored for now.
On 2016/06/20 11:21:39, rmcilroy wrote:
> /s/they are ignored for now/instead we force the stack check's expression
> position which eliminates the empty statement's position/ (or something
similar)

Done.

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/2048203002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:95: void MakeInvalidEqualTo(const
BytecodeSourceInfo& other) {
On 2016/06/20 11:21:39, rmcilroy wrote:
> I'm not keen on this name.
> 
> One possible suggestion to avoid the need for this function - how about in
> BytecodeArrayBuilder, instead of having a AttachSourceInfo, have a
> GetSourcePositionInfo function which returns either the current source
position
> or a invalid one, and pass it directly to the BytecodeNode constructor in
> BytecodeArrayBuilder::Output? I think the compiler should optimize away the
> extra construct/copy. WDYT?
> 
> Just noticed you have other uses of this outside BytecodeArrayBuilder. I think
> the above is still possibly worth while, however I guess you still need this
> function either way. As such, could we rename it to Clone() or something
> similar?

Okay, the method is renamed Clone(). I totally agree the existing name wasn't
great, was trying to convey overwriting uninitialized state.

GetSourcePositionInfo() is going to need the bytecode as an argument, not sure
it's a win either way. If you feel stronger about it, not problem with adding
it.

[rmcilroy, 2016-06-21 10:55:50]

LGTM, thanks.

[oth, 2016-06-21 11:09:08]

The CQ bit was checked by oth@chromium.org

[commit-bot: I haz the power, 2016-06-21 11:09:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2048203002/180001

[commit-bot: I haz the power, 2016-06-21 11:11:23]

Description was changed from

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.
    
Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.
    
Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-06-21 11:11:24]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-21 11:12:40]

Description was changed from

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.
    
Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Minor clean-up of BytecodeSourceInfo.

Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/303d340f669a8521708357432bfe2cb368d42b2f
Cr-Commit-Position: refs/heads/master@{#37136}
==========

[commit-bot: I haz the power, 2016-06-21 11:12:41]

Patchset 10 (id:??) landed as
https://crrev.com/303d340f669a8521708357432bfe2cb368d42b2f
Cr-Commit-Position: refs/heads/master@{#37136}