r215471 - Improve -Wuninitialized to catch const classes being used in their own copy

Richard Smith richard at metafoo.co.uk
Tue Aug 12 16:19:48 PDT 2014 

On Tue, Aug 12, 2014 at 2:05 PM, Richard Trieu <rtrieu at google.com> wrote:

> Author: rtrieu
> Date: Tue Aug 12 16:05:04 2014
> New Revision: 215471
>
> URL: http://llvm.org/viewvc/llvm-project?rev=215471&view=rev
> Log:
> Improve -Wuninitialized to catch const classes being used in their own copy
> constructors.
>
> Modified:
>     cfe/trunk/lib/Sema/SemaDecl.cpp
>     cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>     cfe/trunk/test/SemaCXX/uninitialized.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=215471&r1=215470&r2=215471&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Aug 12 16:05:04 2014
> @@ -8276,6 +8276,15 @@ namespace {
>
>      void VisitObjCMessageExpr(ObjCMessageExpr *E) { return; }
>
> +    void VisitCXXConstructExpr(CXXConstructExpr *E) {
> +      if (E->getConstructor()->isCopyConstructor()) {
>

Should this also apply to move constructors? I'd think it'd help in cases
like:

  void f(string foo_) {
    string foo(std::move(foo)); // oops

... or

  S::S(T x) : x_(x_) {} // oops


> +        if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E->getArg(0))) {
> +          HandleDeclRefExpr(DRE);
> +        }
> +      }
> +      Inherited::VisitCXXConstructExpr(E);
> +    }
> +
>      void HandleDeclRefExpr(DeclRefExpr *DRE) {
>        Decl* ReferenceDecl = DRE->getDecl();
>        if (OrigDecl != ReferenceDecl) return;
>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=215471&r1=215470&r2=215471&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Tue Aug 12 16:05:04 2014
> @@ -2314,12 +2314,18 @@ namespace {
>      }
>
>      void VisitCXXConstructExpr(CXXConstructExpr *E) {
> -      if (E->getConstructor()->isCopyConstructor())
> -        if (ImplicitCastExpr* ICE =
> dyn_cast<ImplicitCastExpr>(E->getArg(0)))
> -          if (ICE->getCastKind() == CK_NoOp)
> -            if (MemberExpr *ME = dyn_cast<MemberExpr>(ICE->getSubExpr()))
> -              HandleMemberExpr(ME, false /*CheckReferenceOnly*/);
> -
> +      if (E->getConstructor()->isCopyConstructor()) {
> +        Expr *ArgExpr = E->getArg(0);
> +        if (ImplicitCastExpr* ICE = dyn_cast<ImplicitCastExpr>(ArgExpr)) {
> +          if (ICE->getCastKind() == CK_NoOp) {
> +            ArgExpr = ICE->getSubExpr();
> +          }
> +        }
>

We should probably IgnoreParenImpCasts() or similar here. Trying to exactly
match the shape of implicit AST nodes is bound to be bug-prone.

+
> +        if (MemberExpr *ME = dyn_cast<MemberExpr>(ArgExpr)) {
> +          HandleMemberExpr(ME, false /*CheckReferenceOnly*/);
> +        }
> +      }
>        Inherited::VisitCXXConstructExpr(E);
>      }
>
>
> Modified: cfe/trunk/test/SemaCXX/uninitialized.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/uninitialized.cpp?rev=215471&r1=215470&r2=215471&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/uninitialized.cpp (original)
> +++ cfe/trunk/test/SemaCXX/uninitialized.cpp Tue Aug 12 16:05:04 2014
> @@ -127,6 +127,9 @@ void setupA(bool x) {
>    A *a26 = new A(a26->get());    // expected-warning {{variable 'a26' is
> uninitialized when used within its own initialization}}
>    A *a27 = new A(a27->get2());  // expected-warning {{variable 'a27' is
> uninitialized when used within its own initialization}}
>    A *a28 = new A(a28->num);  // expected-warning {{variable 'a28' is
> uninitialized when used within its own initialization}}
> +
> +  const A a29(a29);  // expected-warning {{variable 'a29' is
> uninitialized when used within its own initialization}}
> +  const A a30 = a30;  // expected-warning {{variable 'a30' is
> uninitialized when used within its own initialization}}
>  }
>
>  bool x;
> @@ -163,6 +166,9 @@ A *a26 = new A(a26->get());    // expect
>  A *a27 = new A(a27->get2());  // expected-warning {{variable 'a27' is
> uninitialized when used within its own initialization}}
>  A *a28 = new A(a28->num);  // expected-warning {{variable 'a28' is
> uninitialized when used within its own initialization}}
>
> +const A a29(a29);  // expected-warning {{variable 'a29' is uninitialized
> when used within its own initialization}}
> +const A a30 = a30;  // expected-warning {{variable 'a30' is uninitialized
> when used within its own initialization}}
> +
>  struct B {
>    // POD struct.
>    int x;
> @@ -209,6 +215,10 @@ void setupB() {
>
>    B b17 = { b17.x = 5, b17.y = 0 };
>    B b18 = { b18.x + 1, b18.y };  // expected-warning 2{{variable 'b18' is
> uninitialized when used within its own initialization}}
> +
> +  const B b19 = b19;  // expected-warning {{variable 'b19' is
> uninitialized when used within its own initialization}}
> +  const B b20(b20);  // expected-warning {{variable 'b20' is
> uninitialized when used within its own initialization}}
> +
>  }
>
>  B b1;
> @@ -234,6 +244,8 @@ B* b16 = getPtrB(b16->y);  // expected-w
>  B b17 = { b17.x = 5, b17.y = 0 };
>  B b18 = { b18.x + 1, b18.y };  // expected-warning 2{{variable 'b18' is
> uninitialized when used within its own initialization}}
>
> +const B b19 = b19;  // expected-warning {{variable 'b19' is uninitialized
> when used within its own initialization}}
> +const B b20(b20);  // expected-warning {{variable 'b20' is uninitialized
> when used within its own initialization}}
>
>  // Also test similar constructs in a field's initializer.
>  struct S {
> @@ -385,6 +397,11 @@ namespace {
>      G(char (*)[7]) : f3(f3->*f_ptr) {} // expected-warning {{field 'f3'
> is uninitialized when used here}}
>      G(char (*)[8]) : f3(new F(f3->*ptr)) {} // expected-warning {{field
> 'f3' is uninitialized when used here}}
>    };
> +
> +  struct H {
> +    H() : a(a) {}  // expected-warning {{field 'a' is uninitialized when
> used here}}
> +    const A a;
> +  };
>  }
>
>  namespace statics {
> @@ -555,7 +572,7 @@ namespace record_fields {
>      B(char (*)[9]) : a(normal(a)) {}  // expected-warning
> {{uninitialized}}
>    };
>    struct C {
> -    C() {} // expected-note4{{in this constructor}}
> +    C() {} // expected-note5{{in this constructor}}
>      A a1 = a1;  // expected-warning {{uninitialized}}
>      A a2 = a2.get();  // expected-warning {{uninitialized}}
>      A a3 = a3.num();
> @@ -565,8 +582,9 @@ namespace record_fields {
>      A a7 = const_ref(a7);
>      A a8 = pointer(&a8);
>      A a9 = normal(a9);  // expected-warning {{uninitialized}}
> +    const A a10 = a10;  // expected-warning {{uninitialized}}
>    };
> -  struct D {  // expected-note4{{in the implicit default constructor}}
> +  struct D {  // expected-note5{{in the implicit default constructor}}
>      A a1 = a1;  // expected-warning {{uninitialized}}
>      A a2 = a2.get();  // expected-warning {{uninitialized}}
>      A a3 = a3.num();
> @@ -576,6 +594,7 @@ namespace record_fields {
>      A a7 = const_ref(a7);
>      A a8 = pointer(&a8);
>      A a9 = normal(a9);  // expected-warning {{uninitialized}}
> +    const A a10 = a10;  // expected-warning {{uninitialized}}
>    };
>    D d;
>    struct E {
> @@ -588,6 +607,7 @@ namespace record_fields {
>      A a7 = const_ref(a7);
>      A a8 = pointer(&a8);
>      A a9 = normal(a9);
> +    const A a10 = a10;
>    };
>  }
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140812/a337de53/attachment.html>

More information about the cfe-commits mailing list