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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
