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!
> 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.
> + 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).
> + 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.)
> +
> + // 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
