[PATCH] D137070: [clang][Interp] Support destructors

[Aaron Ballman via Phabricator via cfe-commits]

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