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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:199-200
+bool ByteCodeStmtGen<Emitter>::visitUnscopedCompoundStmt(const Stmt *S) {
+  if (isa<NullStmt>(S))
+    return true;
+
----------------
aaron.ballman wrote:
> Errr, I'm surprised it isn't UB to call this with anything but a `CompoundStmt` given the function name.
Yeah, I'm not too happy about that either. I'll see what I can do.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:209
+
+  return this->visitStmt(S);
+}
----------------
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?)


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:324-328
+  LocalScope<Emitter> Scope(this);
+  if (!this->visitUnscopedCompoundStmt(Body))
     return false;
+
+  Scope.emitDestructors();
----------------
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.


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

https://reviews.llvm.org/D145545