aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1900-1902
+/// When calling this, we have a pointer of the local-to-destroy
+/// on the stack.
+/// Emit destruction of record types (or arrays of record types).
----------------
As a FIXME: you should also handle virtual destructors at some point, whenever
you get around to handling virtual functions in general.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1915-1916
+
+ if (const CXXDestructorDecl *Dtor = ElemRecord->getDestructor();
+ Dtor && !Dtor->isTrivial()) {
+ for (size_t I = 0, E = Desc->getNumElems(); I != E; ++I) {
----------------
`isTrivial()` only works once the class has been fully built up by Sema IIRC;
we should have a test case for that situation.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1917
+ Dtor && !Dtor->isTrivial()) {
+ for (size_t I = 0, E = Desc->getNumElems(); I != E; ++I) {
+ if (!this->emitConstUint64(I, SourceInfo{}))
----------------
This looks like it will destroy the array elements in order instead of in
reverse order -- need test coverage for that. See
https://eel.is/c++draft/class.dtor#13.sentence-5
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1934
+ const Descriptor *D = Field.Desc;
+ if (!D->isPrimitive() && !D->isPrimitiveArray()) {
+ if (!this->emitDupPtr(SourceInfo{}))
----------------
Won't this also destroy static data members? (Needs a test case for that.)
Also, what if the record is a union and not a structure? We only want to
destroy the active member in that case, not all of the variant members, right?
(Also needs a test case.)
See http://eel.is/c++draft/class.dtor#13
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1958-1963
+ for (const Record::Base &Base : llvm::reverse(R->bases())) {
+ if (!this->emitGetPtrBase(Base.Offset, SourceInfo{}))
+ return false;
+ if (!this->emitRecordDestruction(Base.Desc))
+ return false;
+ }
----------------
I don't think we handle virtual functions yet so I doubt we handle virtual
bases, but that's something that might need a FIXME comment.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:345
- ~LocalScope() override { this->emitDestruction(); }
+ virtual ~LocalScope() override { this->emitDestruction(); }
----------------
No need to add the `virtual` here as the `override` already signals that (can't
override a non-virtual function).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137070/new/
https://reviews.llvm.org/D137070
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
