[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/ByteCodeStmtGen.cpp:209
+
+  return this->visitStmt(S);
+}
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > 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.
> I think you're overthinking this, this just for the `!isa<CompoundStmt>(S)` case (which IIRC happens at least for comma operator bin ops?)
Oh! Yeah, I totally overthought that. I got tripped up by this accepting things other than a `CompoundStmt` because of the function name. Sorry for the noise!


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:324-328
+  LocalScope<Emitter> Scope(this);
+  if (!this->visitUnscopedCompoundStmt(Body))
     return false;
+
+  Scope.emitDestructors();
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > `AutoScope` and some curly braces to delimit the scope object lifetime?
> The problem is that we want to emit the destructors before the jump, but not destroy the scope. That should happen after the end label, when the loop is finished altogether so an `AutoScope` doesn't work.
Ahh yeah, that's a good point... but this still worries me a bit because of how easy it is to misuse the scope after emitting the destructors. Notionally, we want two scopes, right? One scope for the loop construct guts and a second (inner) scope for the loop body. That's how the construct is modeled in the standard: http://eel.is/c++draft/stmt.while#2


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

https://reviews.llvm.org/D145545