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