aaron.ballman accepted this revision.
This revision is now accepted and ready to land.
================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:284
@@ -283,2 +283,3 @@
varDecl(isDefinition(), HasDefaultConstructor,
+ hasAutomaticStorageDuration(),
hasType(recordDecl(has(fieldDecl()),
----------------
alexfh wrote:
> 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.
Ah, that's a very valid point.
http://reviews.llvm.org/D19672
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
