[PATCH] D145545: [clang][Interp] Fix local variable (destructor) management in loop bodies

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:338
+    this->emitDestructors();
+    this->Ctx->emitDestroy(*Idx, SourceInfo{});
+  }
----------------
Should we be setting `Idx = std::nullopt;` after this so that the `LocalScope` destructor does not also emit a destroy for the same `Idx`?


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:199-200
+bool ByteCodeStmtGen<Emitter>::visitUnscopedCompoundStmt(const Stmt *S) {
+  if (isa<NullStmt>(S))
+    return true;
+
----------------
Errr, I'm surprised it isn't UB to call this with anything but a `CompoundStmt` given the function name.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:209
+
+  return this->visitStmt(S);
+}
----------------
It's a bit of a surprise that we visit the entire body of the compound statement before we visit the compound statement itself. I was thinking the compound statement could potentially have prologue work it needs to do, but now I'm second-guessing that. But this design still feels a little bit off... it's basically doing a post-order traversal just for this one node, and I wonder if we want something more general like a `preVisitFoo()` followed by `visitFoo()` followed by `postVisitFoo()` that applies to any AST node.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:324-328
+  LocalScope<Emitter> Scope(this);
+  if (!this->visitUnscopedCompoundStmt(Body))
     return false;
+
+  Scope.emitDestructors();
----------------
`AutoScope` and some curly braces to delimit the scope object lifetime?


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:345
   LoopScope<Emitter> LS(this, EndLabel, CondLabel);
+  LocalScope<Emitter> Scope(this);
 
----------------
Similar question here and elsewhere. The concern I have with this form is that it's pretty easy to accidentally miss that we've emitted the destructors in later code, whereas using the RAII object makes that impossible.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145545/new/

https://reviews.llvm.org/D145545