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