[kernel] Offsets on loops

runtime/vm/kernel_binary_flowgraph.cc

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/kernel_binary_flowgraph.h"
 
 #include "vm/compiler.h"
 #include "vm/longjump.h"
 #include "vm/object_store.h"
 
@@ skipping 742 matching lines @@
     }
     case kLabeledStatement:
       VisitStatement();  // read body.
       return;
     case kBreakStatement:
       builder_->ReadPosition();  // read position.
       builder_->ReadUInt();      // read target_index.
       return;
     case kWhileStatement:
       ++depth_.loop_;
+      builder_->ReadPosition();  // read position.
       VisitExpression();  // read condition.
       VisitStatement();   // read body.
       --depth_.loop_;
       return;
     case kDoStatement:
       ++depth_.loop_;
+      builder_->ReadPosition();  // read position.
       VisitStatement();   // read body.
       VisitExpression();  // read condition.
       --depth_.loop_;
       return;
     case kForStatement: {
       PositionScope scope(builder_->reader_);
 
       intptr_t offset =
           builder_->ReaderOffset() - 1;  // -1 to include tag byte.
 
       EnterScope(offset);
 
+      TokenPosition position = builder_->ReadPosition();  // read position.
       intptr_t list_length =
           builder_->ReadListLength();  // read number of variables.
       for (intptr_t i = 0; i < list_length; ++i) {
         VisitVariableDeclaration();  // read ith variable.
       }
 
       ++depth_.loop_;
 
       Tag tag = builder_->ReadTag();  // Read first part of condition.
       if (tag == kSomething) {
         VisitExpression();  // read rest of condition.
       }
       list_length = builder_->ReadListLength();  // read number of updates.
       for (intptr_t i = 0; i < list_length; ++i) {
         VisitExpression();  // read ith update.
       }
       VisitStatement();  // read body.
 
       --depth_.loop_;
 
-      ExitScope(builder_->reader_->min_position(),
-                builder_->reader_->max_position());
+      ExitScope(position, builder_->reader_->max_position());
       return;
     }
     case kForInStatement:
     case kAsyncForInStatement: {
       PositionScope scope(builder_->reader_);
 
       intptr_t start_offset =
           builder_->ReaderOffset() - 1;  // -1 to include tag byte.
 
-      TokenPosition position = builder_->ReadPosition();  // read position.
+      builder_->ReadPosition();  // read position.
+      TokenPosition body_position =
+          builder_->ReadPosition();  // read body position.
 
       // Notice the ordering: We skip the variable, read the iterable, go back,
       // re-read the variable, go forward to after having read the iterable.
       intptr_t offset = builder_->ReaderOffset();
       builder_->SkipVariableDeclaration();  // read variable.
       VisitExpression();                    // read iterable.
 
       ++depth_.for_in_;
       AddIteratorVariable();
       ++depth_.loop_;
       EnterScope(start_offset);
 
       {
         AlternativeReadingScope alt(builder_->reader_, offset);
         VisitVariableDeclaration();  // read variable.
       }
       VisitStatement();  // read body.
 
-      if (!position.IsReal()) {
-        position = builder_->reader_->min_position();
+      if (!body_position.IsReal()) {
+        body_position = builder_->reader_->min_position();
       }
       // TODO(jensj): From kernel_binary.cc
       // forinstmt->variable_->set_end_position(forinstmt->position_);
-      ExitScope(position, builder_->reader_->max_position());
+      ExitScope(body_position, builder_->reader_->max_position());
       --depth_.loop_;
       --depth_.for_in_;
       return;
     }
     case kSwitchStatement: {
       AddSwitchVariable();
       VisitExpression();                            // read condition.
       int case_count = builder_->ReadListLength();  // read number of cases.
       for (intptr_t i = 0; i < case_count; ++i) {
         int expression_count =
@@ skipping 3250 matching lines @@
       return;
     }
     case kLabeledStatement:
       SkipStatement();  // read body.
       return;
     case kBreakStatement:
       ReadPosition();  // read position.
       ReadUInt();      // read target_index.
       return;
     case kWhileStatement:
+      ReadPosition();    // read position.

[ahe, 2017/08/08 11:38:51]

print("Hello, World!"); // Prints "Hello, World!"

That's how you add comments to an example used in introducing students to
programming.

Repeating exactly the name of the method isn't a useful comment and should be
avoided.

       SkipExpression();  // read condition.

[ahe, 2017/08/08 11:38:51]

Similarly, adding a comment that contradicts with the code isn't useful, and
should be removed.

       SkipStatement();   // read body.
       return;
     case kDoStatement:
+      ReadPosition();    // read position.
       SkipStatement();   // read body.
       SkipExpression();  // read condition.
       return;
     case kForStatement: {
+      ReadPosition();                    // read position.
       SkipListOfVariableDeclarations();  // read variables.
       Tag tag = ReadTag();               // Read first part of condition.

[ahe, 2017/08/08 11:38:51]

Generally, comments should start with an uppercase letter as here. And contain
information that isn't obvious from the code.

However, this code could be made self explanatory:

Tag conditionStart = ReadTag();

       if (tag == kSomething) {
         SkipExpression();  // read rest of condition.
       }
       SkipListOfExpressions();  // read updates.
       SkipStatement();          // read body.
       return;
     }
     case kForInStatement:
     case kAsyncForInStatement:
       ReadPosition();             // read position.

[ahe, 2017/08/08 11:38:51]

Read position of what?

+      ReadPosition();             // read body position.

[ahe, 2017/08/08 11:38:51]

This comment adds value as it adds additional information (that it's the
position of the *body*).

       SkipVariableDeclaration();  // read variable.
       SkipExpression();           // read iterable.
       SkipStatement();            // read body.
       return;
     case kSwitchStatement: {
       SkipExpression();                   // read condition.
       int case_count = ReadListLength();  // read number of cases.
       for (intptr_t i = 0; i < case_count; ++i) {
         int expression_count = ReadListLength();  // read number of expressions.
         for (intptr_t j = 0; j < expression_count; ++j) {
@@ skipping 1945 matching lines @@
     if (NeedsDebugStepCheck(parsed_function()->function(), position)) {
       instructions += DebugStepCheck(position);
     }
     instructions += Goto(destination);
   }
   return instructions;
 }
 
 Fragment StreamingFlowGraphBuilder::BuildWhileStatement() {
   loop_depth_inc();
+  ReadPosition();  // read position.
+
   bool negate;
   Fragment condition = TranslateCondition(&negate);  // read condition.
   TargetEntryInstr* body_entry;
   TargetEntryInstr* loop_exit;
   condition += BranchIfTrue(&body_entry, &loop_exit, negate);
 
   Fragment body(body_entry);
   body += BuildStatement();  // read body.
 
   Instruction* entry;
   if (body.is_open()) {
     JoinEntryInstr* join = BuildJoinEntry();
     body += Goto(join);
 
     Fragment loop(join);
     loop += CheckStackOverflow();
     loop += condition;
     entry = new (Z) GotoInstr(join, Thread::Current()->GetNextDeoptId());
   } else {
     entry = condition.entry;
   }
 
   loop_depth_dec();
   return Fragment(entry, loop_exit);
 }
 
 Fragment StreamingFlowGraphBuilder::BuildDoStatement() {
   loop_depth_inc();
+  ReadPosition();                    // read position.
   Fragment body = BuildStatement();  // read body.
 
   if (body.is_closed()) {
     SkipExpression();  // read condition.
     loop_depth_dec();
     return body;
   }
 
   bool negate;
   JoinEntryInstr* join = BuildJoinEntry();
   Fragment loop(join);
   loop += CheckStackOverflow();
   loop += body;
   loop += TranslateCondition(&negate);  // read condition.
   TargetEntryInstr* loop_repeat;
   TargetEntryInstr* loop_exit;
   loop += BranchIfTrue(&loop_repeat, &loop_exit, negate);
 
   Fragment repeat(loop_repeat);
   repeat += Goto(join);
 
   loop_depth_dec();
   return Fragment(new (Z) GotoInstr(join, Thread::Current()->GetNextDeoptId()),
                   loop_exit);
 }
 
 Fragment StreamingFlowGraphBuilder::BuildForStatement() {
   intptr_t offset = ReaderOffset() - 1;  // Include the tag.
 
+  ReadPosition();  // read position.
+
   Fragment declarations;
 
   bool new_context = false;
   declarations += EnterScope(offset, &new_context);
 
   intptr_t list_length = ReadListLength();  // read number of variables.
   for (intptr_t i = 0; i < list_length; ++i) {
     declarations += BuildVariableDeclaration();  // read ith variable.
   }
 
@@ skipping 41 matching lines @@
   loop_depth_dec();
 
   loop += ExitScope(offset);
 
   return loop;
 }
 
 Fragment StreamingFlowGraphBuilder::BuildForInStatement(bool async) {
   intptr_t offset = ReaderOffset() - 1;  // Include the tag.
 
-  TokenPosition position = ReadPosition();  // read position.
+  ReadPosition();                                // read position.

[ahe, 2017/08/08 11:38:51]

Of what?

+  TokenPosition body_position = ReadPosition();  // read body position.
   intptr_t variable_kernel_position = ReaderOffset();
   SkipVariableDeclaration();  // read variable.
 
   TokenPosition iterable_position = TokenPosition::kNoSource;
   Fragment instructions =
       BuildExpression(&iterable_position);  // read iterable.
   instructions += PushArgument();
 
   const dart::String& iterator_getter = dart::String::ZoneHandle(
       Z, dart::Field::GetterSymbol(Symbols::Iterator()));
@@ skipping 12 matching lines @@
   TargetEntryInstr* body_entry;
   TargetEntryInstr* loop_exit;
   condition += BranchIfTrue(&body_entry, &loop_exit, false);
 
   Fragment body(body_entry);
   body += EnterScope(offset);
   body += LoadLocal(iterator);
   body += PushArgument();
   const dart::String& current_getter = dart::String::ZoneHandle(
       Z, dart::Field::GetterSymbol(Symbols::Current()));
-  body += InstanceCall(position, current_getter, Token::kGET, 1);
+  body += InstanceCall(body_position, current_getter, Token::kGET, 1);
   body += StoreLocal(TokenPosition::kNoSource,
                      LookupVariable(variable_kernel_position));
   body += Drop();
   body += BuildStatement();  // read body.
   body += ExitScope(offset);
 
   if (body.is_open()) {
     JoinEntryInstr* join = BuildJoinEntry();
     instructions += Goto(join);
     body += Goto(join);
@@ skipping 1060 matching lines @@
     }
   }
 
   return Array::Handle(Array::null());
 }
 
 }  // namespace kernel
 }  // namespace dart
 
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)