[PATCH] Improve -Winvalid-noreturn
Richard Trieu
rtrieu at google.com
Wed May 27 17:22:38 PDT 2015
On Wed, May 27, 2015 at 6:30 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> 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!
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150527/a5569227/attachment.html>
More information about the cfe-commits
mailing list