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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
