[interpreter] Filter expression positions at source.

src/interpreter/bytecode-array-builder.cc

Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc b/src/interpreter/bytecode-array-builder.cc
index fa57c16c7cca85c9faa418815cddf63dd0620b3e..39435b286996915f815e8eb014b3c7a6f78e980f 100644
--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -81,8 +81,11 @@ Handle<BytecodeArray> BytecodeArrayBuilder::ToBytecodeArray() {
 
 void BytecodeArrayBuilder::AttachSourceInfo(BytecodeNode* node) {
   if (latest_source_info_.is_valid()) {
-    node->source_info().Update(latest_source_info_);
-    latest_source_info_.set_invalid();
+    if (latest_source_info_.is_statement() || !FLAG_ignition_filter_positions ||
+        !Bytecodes::IsWithoutExternalSideEffects(node->bytecode())) {

[rmcilroy, 2016/06/07 10:21:41]

Please pull out a CanFilterPosition function for clarity

[oth, 2016/06/08 09:19:41]

Added NeedExpressionPosition(Bytecode).

+      node->source_info() = latest_source_info_;
+      latest_source_info_.set_invalid();

[rmcilroy, 2016/06/07 10:21:41]

I think you should set_invalid unconditionally, otherwise you might attach it to
a later node.

[oth, 2016/06/08 09:19:41]

The goal is to defer writing the expression position until we discover it's
needed. If we invalidate it unconditionally, it can't be applied to where it's
needed later, e.g.

E > Star r3
    LdaGlobal ...

Should become:

    Star r3
E > LdaGlobal ...

Not:

    Star r3
    LdaGlobal ...

[rmcilroy, 2016/06/08 13:45:11]

I see, makes sense. Could you add a comment here as such.

[oth, 2016/06/08 15:04:21]

Done.

+    }
   }
 }
 
@@ -722,6 +725,9 @@ void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {
 
 void BytecodeArrayBuilder::SetExpressionPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
+  if (latest_source_info_.is_expression()) {
+    latest_source_info_.set_invalid();

[rmcilroy, 2016/06/07 10:21:41]

Why do you need to set this as invalid, won't Update  update latest_source_info_
appropriately if both are expressions (and if not, it should).

[oth, 2016/06/08 09:19:41]

Update() does not do the right thing. I don't want to tweak Update here as I'd
like to get rid of it entirely.

[rmcilroy, 2016/06/08 13:45:11]

I'm fine with getting rid of Update (I agree the logic in there is hard to make
sense of), however I'd like to ensure that we keep the logic for whether a
source position should overwrite another one be centralized in one place -
whether that means having a couple of additional functions in SourcePositionInfo
with different semantics or one which takes some additional parameters to
specify the situation.

For this CL, I'm fine with doing this, but please add a comment as to why it is
set invalid and a TODO with how you intend to deal with this when Update
disappears.

[oth, 2016/06/08 15:04:20]

Amen.
Done.

+  }
   latest_source_info_.Update({expr->position(), false});
 }