[PATCH] Improve -Winvalid-noreturn
Aaron Ballman
aaron at aaronballman.com
Wed May 27 06:30:23 PDT 2015
On Tue, May 26, 2015 at 11:16 PM, Richard Trieu <rtrieu at google.com> 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
More information about the cfe-commits
mailing list