On Wed, May 27, 2015 at 6:30 AM, Aaron Ballman <[email protected]> wrote:
> On Tue, May 26, 2015 at 11:16 PM, Richard Trieu <[email protected]> wrote: > > Aaron Ballman brought up a point on the mailing list. > CXXRecordDecl::getDestructor() can return null in some places. This patch > restructures the isAnyDestructorNoReturn() to be on the CXXRecordDecl > instead of on the CXXDestructorDecl. If the destructor does not exist, it > is safe to assume it does not have the noreturn attribute. > > Some minor nits and a question. Should we worry about a pathological case > like: > > struct B { > virtual ~B(); > }; > > struct D : B { > [[noreturn]] ~D(); > }; > > [[noreturn]] void f() { > B *b = new D; > delete b; > } // Does this warn? > > void g() { > B *b = new D; > delete b; > std::cout << "Does this warn about unreachable code?"; > } > > I'm not overly concerned about these cases and any false > positives/negatives, but we may want to add some FIXMEs to the tests > for them. > > Aside from these tiny nits, LGTM! > > Committed in r238382. 'delete' can be used with a null pointer, which is basically a no-op. Without tracking non-null pointers, the warning can't be sure that a delete of any object would definitely call a destructor. I added the delete test case with FIXMEs. > > > Index: include/clang/AST/DeclCXX.h > > =================================================================== > > --- include/clang/AST/DeclCXX.h > > +++ include/clang/AST/DeclCXX.h > > @@ -1392,6 +1392,10 @@ > > /// \brief Returns the destructor decl for this class. > > CXXDestructorDecl *getDestructor() const; > > > > + // Returns true if the class destructor, or any implicitly invoked > > + // destructors are marked noreturn. > > Use Doxygen-style comments, please. > Fixed. > > > + bool isAnyDestructorNoReturn() const; > > + > > /// \brief If the class is a local class [class.local], returns > > /// the enclosing function declaration. > > const FunctionDecl *isLocalClass() const { > > Index: lib/AST/DeclCXX.cpp > > =================================================================== > > --- lib/AST/DeclCXX.cpp > > +++ lib/AST/DeclCXX.cpp > > @@ -1315,6 +1315,30 @@ > > return Dtor; > > } > > > > +bool CXXRecordDecl::isAnyDestructorNoReturn() const { > > + // Destructor is noreturn > > Comments should include punctuation (here and elsewhere). > Fixed. > > > + if (const CXXDestructorDecl *Destructor = getDestructor()) > > + if (Destructor->isNoReturn()) > > + return true; > > + > > + // Check base classes destructor for noreturn > > + for (const auto &Base : bases()) > > + if (Base.getType() > > + ->getAsCXXRecordDecl() > > + ->isAnyDestructorNoReturn()) > > + return true; > > Use std::any_of? (Not required for the LGTM since that's more of a grey > area.) > Skipped. > > > + > > + // Check fields for noreturn > > + for (const auto *Field : fields()) > > + if (const CXXRecordDecl *RD = > > + > Field->getType()->getBaseElementTypeUnsafe()->getAsCXXRecordDecl()) > > + if (RD->isAnyDestructorNoReturn()) > > + return true; > > + > > + // All destructors are not noreturn > > + return false; > > +} > > + > > void CXXRecordDecl::completeDefinition() { > > completeDefinition(nullptr); > > } > > Index: lib/Analysis/CFG.cpp > > =================================================================== > > --- lib/Analysis/CFG.cpp > > +++ lib/Analysis/CFG.cpp > > @@ -1179,8 +1179,7 @@ > > } > > Ty = Context->getBaseElementType(Ty); > > > > - const CXXDestructorDecl *Dtor = > Ty->getAsCXXRecordDecl()->getDestructor(); > > - if (Dtor->isNoReturn()) > > + if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn()) > > Block = createNoReturnBlock(); > > else > > autoCreateBlock(); > > @@ -3682,7 +3681,7 @@ > > > > const CXXDestructorDecl *Dtor = E->getTemporary()->getDestructor(); > > > > - if (Dtor->isNoReturn()) { > > + if (Dtor->getParent()->isAnyDestructorNoReturn()) { > > // If the destructor is marked as a no-return destructor, we need > to > > // create a new block for the destructor which does not have as a > > // successor anything built thus far. Control won't flow out of > this > > Index: test/SemaCXX/attr-noreturn.cpp > > =================================================================== > > --- test/SemaCXX/attr-noreturn.cpp > > +++ test/SemaCXX/attr-noreturn.cpp > > @@ -17,6 +17,132 @@ > > } > > } > > > > +namespace destructor_tests { > > + __attribute__((noreturn)) void fail(); > > + > > + struct A { > > + ~A() __attribute__((noreturn)) { fail(); } > > + }; > > + struct B { > > + B() {} > > + ~B() __attribute__((noreturn)) { fail(); } > > + }; > > + struct C : A {}; > > + struct D : B {}; > > + struct E : virtual A {}; > > + struct F : A, virtual B {}; > > + struct G : E {}; > > + struct H : virtual D {}; > > + struct I : A {}; > > + struct J : I {}; > > + struct K : virtual A {}; > > + struct L : K {}; > > + struct M : virtual C {}; > > + struct N : M {}; > > + struct O { N n; }; > > + > > + __attribute__((noreturn)) void test_1() { A a; } > > + __attribute__((noreturn)) void test_2() { B b; } > > + __attribute__((noreturn)) void test_3() { C c; } > > + __attribute__((noreturn)) void test_4() { D d; } > > + __attribute__((noreturn)) void test_5() { E e; } > > + __attribute__((noreturn)) void test_6() { F f; } > > + __attribute__((noreturn)) void test_7() { G g; } > > + __attribute__((noreturn)) void test_8() { H h; } > > + __attribute__((noreturn)) void test_9() { I i; } > > + __attribute__((noreturn)) void test_10() { J j; } > > + __attribute__((noreturn)) void test_11() { K k; } > > + __attribute__((noreturn)) void test_12() { L l; } > > + __attribute__((noreturn)) void test_13() { M m; } > > + __attribute__((noreturn)) void test_14() { N n; } > > + __attribute__((noreturn)) void test_15() { O o; } > > + > > + __attribute__((noreturn)) void test_16() { const A& a = A(); } > > + __attribute__((noreturn)) void test_17() { const B& b = B(); } > > + __attribute__((noreturn)) void test_18() { const C& c = C(); } > > + __attribute__((noreturn)) void test_19() { const D& d = D(); } > > + __attribute__((noreturn)) void test_20() { const E& e = E(); } > > + __attribute__((noreturn)) void test_21() { const F& f = F(); } > > + __attribute__((noreturn)) void test_22() { const G& g = G(); } > > + __attribute__((noreturn)) void test_23() { const H& h = H(); } > > + __attribute__((noreturn)) void test_24() { const I& i = I(); } > > + __attribute__((noreturn)) void test_25() { const J& j = J(); } > > + __attribute__((noreturn)) void test_26() { const K& k = K(); } > > + __attribute__((noreturn)) void test_27() { const L& l = L(); } > > + __attribute__((noreturn)) void test_28() { const M& m = M(); } > > + __attribute__((noreturn)) void test_29() { const N& n = N(); } > > + __attribute__((noreturn)) void test_30() { const O& o = O(); } > > + > > + struct AA {}; > > + struct BB { BB() {} ~BB() {} }; > > + struct CC : AA {}; > > + struct DD : BB {}; > > + struct EE : virtual AA {}; > > + struct FF : AA, virtual BB {}; > > + struct GG : EE {}; > > + struct HH : virtual DD {}; > > + struct II : AA {}; > > + struct JJ : II {}; > > + struct KK : virtual AA {}; > > + struct LL : KK {}; > > + struct MM : virtual CC {}; > > + struct NN : MM {}; > > + struct OO { NN n; }; > > + > > + __attribute__((noreturn)) void test_31() { > > + AA a; > > + BB b; > > + CC c; > > + DD d; > > + EE e; > > + FF f; > > + GG g; > > + HH h; > > + II i; > > + JJ j; > > + KK k; > > + LL l; > > + MM m; > > + NN n; > > + OO o; > > + > > + const AA& aa = AA(); > > + const BB& bb = BB(); > > + const CC& cc = CC(); > > + const DD& dd = DD(); > > + const EE& ee = EE(); > > + const FF& ff = FF(); > > + const GG& gg = GG(); > > + const HH& hh = HH(); > > + const II& ii = II(); > > + const JJ& jj = JJ(); > > + const KK& kk = KK(); > > + const LL& ll = LL(); > > + const MM& mm = MM(); > > + const NN& nn = NN(); > > + const OO& oo = OO(); > > + } // expected-warning {{function declared 'noreturn' should not > return}} > > + > > + struct P { > > + ~P() __attribute__((noreturn)) { fail(); } > > + void foo() {} > > + }; > > + struct Q : P { }; > > + __attribute__((noreturn)) void test31() { > > + P().foo(); > > + } > > + __attribute__((noreturn)) void test32() { > > + Q().foo(); > > + } > > + > > + struct R { > > + A a[5]; > > + }; > > + __attribute__((noreturn)) void test33() { > > + R r; > > + } > > +} > > + > > // PR5620 > > void f0() __attribute__((__noreturn__)); > > void f1(void (*)()); > > ~Aaron >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
