[PATCH] D19672: [clang-tidy] cppcoreguidelines-pro-type-member-init should not complain about static variables

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:284
@@ -283,2 +283,3 @@
       varDecl(isDefinition(), HasDefaultConstructor,
+              hasAutomaticStorageDuration(),
               hasType(recordDecl(has(fieldDecl()),
----------------
aaron.ballman wrote:
> michael_miller wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > We should add a test that this still diagnoses variables with dynamic storage duration. Alternatively, this could be `unless(anyOf(hasStaticStorageDuration(), hasThreadStorageDuration()))`
> > > I don't think it ever diagnosed on the dynamic storage duration, since it doesn't match `cxxNewExpr` ;)
> > Perhaps unrelated but a new expression without the parens wouldn't value-initialize a record type with a default constructor and potentially be a problematic. Maybe we should be checking cxxNewExpr.
> Shouldn't it have diagnosed:
> ```
> struct s {
>   int *i;
>   s() {}
> };
> ```
> ?
This code does not introduce an object with dynamic storage duration. Variable `i` is just a member variable, and the fact that it's a pointer doesn't change much. The check used to warn in this case and continues to do so:

  /tmp/q.cc:3:3: warning: constructor does not initialize these fields: i [cppcoreguidelines-pro-type-member-init]
    s() {}
    ^
        : i()

To resolve the confusion:

[basic.stc]p2
| Static, thread, and automatic storage durations are associated with objects introduced by declarations (3.1) and implicitly created by the implementation (12.2). The dynamic storage duration is associated with objects created with operator new (5.3.4).

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:284
@@ -283,2 +283,3 @@
       varDecl(isDefinition(), HasDefaultConstructor,
+              hasAutomaticStorageDuration(),
               hasType(recordDecl(has(fieldDecl()),
----------------
alexfh wrote:
> aaron.ballman wrote:
> > michael_miller wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > We should add a test that this still diagnoses variables with dynamic storage duration. Alternatively, this could be `unless(anyOf(hasStaticStorageDuration(), hasThreadStorageDuration()))`
> > > > I don't think it ever diagnosed on the dynamic storage duration, since it doesn't match `cxxNewExpr` ;)
> > > Perhaps unrelated but a new expression without the parens wouldn't value-initialize a record type with a default constructor and potentially be a problematic. Maybe we should be checking cxxNewExpr.
> > Shouldn't it have diagnosed:
> > ```
> > struct s {
> >   int *i;
> >   s() {}
> > };
> > ```
> > ?
> This code does not introduce an object with dynamic storage duration. Variable `i` is just a member variable, and the fact that it's a pointer doesn't change much. The check used to warn in this case and continues to do so:
> 
>   /tmp/q.cc:3:3: warning: constructor does not initialize these fields: i [cppcoreguidelines-pro-type-member-init]
>     s() {}
>     ^
>         : i()
> 
> To resolve the confusion:
> 
> [basic.stc]p2
> | Static, thread, and automatic storage durations are associated with objects introduced by declarations (3.1) and implicitly created by the implementation (12.2). The dynamic storage duration is associated with objects created with operator new (5.3.4).
Michael, we should be checking `cxxNewExpr`, but I would leave this to you or someone else. In this patch I'm just trying to make the check less noisy.


http://reviews.llvm.org/D19672