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

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

tbaeder marked 3 inline comments as done.
tbaeder added inline comments.


================
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) {
----------------
aaron.ballman wrote:
> `isTrivial()` only works once the class has been fully built up by Sema IIRC; we should have a test case for that situation.
Are you saying that `isTrivial()` cannot be used like this, or just that it can, but needs a test case to ensure that this is true?

Also, how would such a test case look like?


================
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{}))
----------------
aaron.ballman wrote:
> 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
Right, that makes a lot of sense, good catch.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1934
+    const Descriptor *D = Field.Desc;
+    if (!D->isPrimitive() && !D->isPrimitiveArray()) {
+      if (!this->emitDupPtr(SourceInfo{}))
----------------
aaron.ballman wrote:
> 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
I saw
```
  // A union destructor does not implicitly destroy its members.
  if (RD->isUnion())
    return true;
```
in `ExprConstant.cpp`, but since we don't handle unions at all in the new interpreter right now, I didn't add anything for them. I added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137070