[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. In D45898#1324625 , @orivej wrote: > I have noticed that this change breaks seemingly valid code: > > class A { protected: ~A(); }; > struct B : A {}; > B f() { return B(); } > B g() { return {}; } > > > `f` compiles, but

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

orivej added a comment. I have noticed that this change breaks seemingly valid code: class A { protected: ~A(); }; struct B : A {}; B f() { return B(); } B g() { return {}; } `f` compiles, but `g` fails with `temporary of type 'A' has protected destructor`. (g++ 8.2 compiles this file.)

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. Yeah, if we weren't exercising the code in our test suite, that would be one thing, but I think a language-mode-specific test probably isn't too valuable. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45898/new/ https://reviews.llvm.org

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

orivej added a comment. Actually, `arc-list-init-destruct.mm` crashes Clang 7 with the same backtrace as this test case, and Clang trunk generates similar assembly (to the the point that I could essentially copy the `// CHECK` comments from the .mm test to a .cpp test), so I'm not sure if addin

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. Always worth adding more tests. Mind writing that one up as a commit? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45898/new/ https://reviews.llvm.org/D45898 ___ cfe-commits mailing list c

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

orivej added a comment. Herald added a subscriber: jkorous. The committed test does not crash Clang 7, but the following test does, yet it compiles without any warnings by the current Clang trunk thanks to this fix. struct A { ~A(); }; struct B : A {}; struct C { C(); B b; }; struct D {

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

This revision was automatically updated to reflect the committed changes. Closed by commit rC341629: [Sema] Check that the destructor for each element of class type is (authored by ahatanak, committed by ). Repository: rC Clang https://reviews.llvm.org/D45898 Files: lib/Sema/SemaInit.cpp

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak updated this revision to Diff 163871. ahatanak marked 2 inline comments as done. ahatanak added a comment. Point the diagnostic at either the relevant init list element or at the close brace. Repository: rC Clang https://reviews.llvm.org/D45898 Files: lib/Sema/SemaInit.cpp test

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added inline comments. Comment at: lib/Sema/SemaInit.cpp:1827-1829 + if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc)) +return false; + return true; rsmith wrote: > Usual Clang convention is to return `true` on error. I also renamed the function to h

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaInit.cpp:1827-1829 + if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc)) +return false; + return true; Usual Clang convention

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak updated this revision to Diff 163612. ahatanak marked 3 inline comments as done. ahatanak added a comment. Move the call that checks the element's destructor into InitListChecker::CheckStructUnionTypes. Repository: rC Clang https://reviews.llvm.org/D45898 Files: lib/Sema/SemaInit

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith added inline comments. Comment at: lib/Sema/SemaInit.cpp:830 + + for (FieldDecl *FD : CXXRD->fields()) +if (auto *CXXRDMember = FD->getType()->getAsCXXRecordDecl()) rsmith wrote: > I think this now checks too much: only those subobjects for which we f

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added a comment. ping Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak updated this revision to Diff 157373. ahatanak added a comment. Check if destructors are accessible from InitListChecker's constructor. Add test cases for designated initializer and brace elision. Repository: rC Clang https://reviews.llvm.org/D45898 Files: lib/Sema/SemaInit.cpp

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith added a comment. Does this check destructors of nested aggregate initializations in the case where brace elision occurs? I don't think just checking the top level of the SK_ListInitialization is enough. Perhaps it would make more sense to mark the dtors used from InitListChecker (in non-

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added a comment. ping Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added a comment. ping Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. LGTM, but I'd like Richard to sign off, too. Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak updated this revision to Diff 153779. ahatanak added a comment. Herald added a subscriber: dexonsmith. Implement the new rule defined here: http://wg21.link/p0968r0#2227. Produce a diagnostic if a class is initialized with aggregate initialization and one of its element's destructor is

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added a comment. In https://reviews.llvm.org/D45898#1104025, @rsmith wrote: > As it happens, the C++ committee fixed the language wording hole here very > recently. The new rule can be found here: http://wg21.link/p0968r0#2227 > In summary: we should to consider the destructor for all

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok. Comment at: test/CodeGenObjCXX/arc-list-init-destruct.mm

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith added a comment. As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227 In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked. @rjmccall Were

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak added a comment. Richard and Doug, do you have any thoughts on John's suggestion? Comment at: test/CodeGenObjCXX/arc-list-init-destruct.mm:1 +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -std=c++1z -fobjc-arc -fobjc-exceptions -fcxx-exceptions -fexceptions -e

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a reviewer: doug.gregor. rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:7074 + if (RD->hasDefinition() && RD->hasNonTrivialDestructor()) +S.MarkFunctionReferenced(E->getExprLoc(), S.LookupDestructor(RD)); + I

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

ahatanak created this revision. ahatanak added reviewers: rsmith, rjmccall. If an initializer in a braced-init-list is a C++ class that has a non-trivial destructor, mark the destructor as referenced. This fixes a crash in CodeGenFunction::destroyCXXObject that occurs when it tries to emit a cal