Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-10 Thread Haojian Wu via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL269024: Fixed cppcoreguidelines-pro-type-member-init when 
checking records with… (authored by hokein).

Changed prior to commit:
  http://reviews.llvm.org/D19993?vs=56439=56668#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19993

Files:
  
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
  
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -27,16 +27,23 @@
 
 namespace {
 
-void fieldsRequiringInit(const RecordDecl::field_range ,
- ASTContext ,
- SmallPtrSetImpl ) {
+// Iterate over all the fields in a record type, both direct and indirect (e.g.
+// if the record contains an anonmyous struct). If OneFieldPerUnion is true and
+// the record type (or indirect field) is a union, forEachField will stop after
+// the first field.
+template 
+void forEachField(const RecordDecl *Record, const T ,
+  bool OneFieldPerUnion, Func &) {
   for (const FieldDecl *F : Fields) {
-if (F->hasInClassInitializer())
-  continue;
-QualType Type = F->getType();
-if (!F->hasInClassInitializer() &&
-utils::type_traits::isTriviallyDefaultConstructible(Type, Context))
-  FieldsToInit.insert(F);
+if (F->isAnonymousStructOrUnion()) {
+  if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
+forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+} else {
+  Fn(F);
+}
+
+if (OneFieldPerUnion && Record->isUnion())
+  break;
   }
 }
 
@@ -179,8 +186,8 @@
   // Gets either the field or base class being initialized by the provided
   // initializer.
   const auto *InitDecl =
-  Init->isMemberInitializer()
-  ? static_cast(Init->getMember())
+  Init->isAnyMemberInitializer()
+  ? static_cast(Init->getAnyMember())
   : Init->getBaseClass()->getAsCXXRecordDecl();
 
   // Add all fields between current field up until the next intializer.
@@ -216,7 +223,8 @@
   Decls.emplace_back(Decl);
 }
   }
-  Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end());
+  forEachField(ClassDecl, ClassDecl->fields(), false,
+   [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template 
@@ -238,22 +246,6 @@
   }
 }
 
-template 
-void forEachField(const RecordDecl *Record, const T ,
-  bool OneFieldPerUnion, Func &) {
-  for (const FieldDecl *F : Fields) {
-if (F->isAnonymousStructOrUnion()) {
-  if (const RecordDecl *R = getCanonicalRecordDecl(F->getType()))
-forEachField(R, R->fields(), OneFieldPerUnion, Fn);
-} else {
-  Fn(F);
-}
-
-if (OneFieldPerUnion && Record->isUnion())
-  break;
-  }
-}
-
 } // anonymous namespace
 
 ProTypeMemberInitCheck::ProTypeMemberInitCheck(StringRef Name,
@@ -314,8 +306,14 @@
   if (IsUnion && ClassDecl->hasInClassInitializer())
 return;
 
+  // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  fieldsRequiringInit(ClassDecl->fields(), Context, FieldsToInit);
+  forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
+if (!F->hasInClassInitializer() &&
+utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
+Context))
+  FieldsToInit.insert(F);
+  });
   if (FieldsToInit.empty())
 return;
 
@@ -325,7 +323,7 @@
 if (Init->isAnyMemberInitializer() && Init->isWritten()) {
   if (IsUnion)
 return; // We can only initialize one member of a union.
-  FieldsToInit.erase(Init->getMember());
+  FieldsToInit.erase(Init->getAnyMember());
 }
   }
   removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -389,8 +387,8 @@
 if (const auto *BaseClassDecl = getCanonicalRecordDecl(Base.getType())) {
   AllBases.emplace_back(BaseClassDecl);
   if (!BaseClassDecl->field_empty() &&
-  utils::type_traits::isTriviallyDefaultConstructible(
-  Base.getType(), Context))
+  utils::type_traits::isTriviallyDefaultConstructible(Base.getType(),
+  Context))
 BasesToInit.insert(BaseClassDecl);
 }
   }
Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
===
---

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-09 Thread Haojian Wu via cfe-commits 
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM.


http://reviews.llvm.org/D19993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-06 Thread Michael Miller via cfe-commits 
michael_miller updated this revision to Diff 56439.
michael_miller added a comment.

Switched to using getAsCXXRecordDecl.


http://reviews.llvm.org/D19993

Files:
  clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
  test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

Index: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
===
--- test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
+++ test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
@@ -357,3 +357,13 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType
   // CHECK-FIXES: PositiveSelfInitialization() : NegativeAggregateType(), PositiveSelfInitialization() {}
 };
+
+class PositiveIndirectMember {
+  struct {
+int *A;
+// CHECK-FIXES: int *A{};
+  };
+
+  PositiveIndirectMember() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+};
Index: test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
===
--- test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
+++ test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
@@ -103,3 +103,14 @@
 
   int X;
 };
+
+class PositiveIndirectMember {
+  struct {
+int *A;
+  };
+
+  PositiveIndirectMember() : A() {}
+  PositiveIndirectMember(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+  // CHECK-FIXES: PositiveIndirectMember(int) : A() {}
+};
Index: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -27,16 +27,23 @@
 
 namespace {
 
-void fieldsRequiringInit(const RecordDecl::field_range ,
- ASTContext ,
- SmallPtrSetImpl ) {
+// Iterate over all the fields in a record type, both direct and indirect (e.g.
+// if the record contains an anonmyous struct). If OneFieldPerUnion is true and
+// the record type (or indirect field) is a union, forEachField will stop after
+// the first field.
+template 
+void forEachField(const RecordDecl *Record, const T ,
+  bool OneFieldPerUnion, Func &) {
   for (const FieldDecl *F : Fields) {
-if (F->hasInClassInitializer())
-  continue;
-QualType Type = F->getType();
-if (!F->hasInClassInitializer() &&
-utils::type_traits::isTriviallyDefaultConstructible(Type, Context))
-  FieldsToInit.insert(F);
+if (F->isAnonymousStructOrUnion()) {
+  if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
+forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+} else {
+  Fn(F);
+}
+
+if (OneFieldPerUnion && Record->isUnion())
+  break;
   }
 }
 
@@ -179,8 +186,8 @@
   // Gets either the field or base class being initialized by the provided
   // initializer.
   const auto *InitDecl =
-  Init->isMemberInitializer()
-  ? static_cast(Init->getMember())
+  Init->isAnyMemberInitializer()
+  ? static_cast(Init->getAnyMember())
   : Init->getBaseClass()->getAsCXXRecordDecl();
 
   // Add all fields between current field up until the next intializer.
@@ -216,7 +223,8 @@
   Decls.emplace_back(Decl);
 }
   }
-  Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end());
+  forEachField(ClassDecl, ClassDecl->fields(), false,
+   [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template 
@@ -238,22 +246,6 @@
   }
 }
 
-template 
-void forEachField(const RecordDecl *Record, const T ,
-  bool OneFieldPerUnion, Func &) {
-  for (const FieldDecl *F : Fields) {
-if (F->isAnonymousStructOrUnion()) {
-  if (const RecordDecl *R = getCanonicalRecordDecl(F->getType()))
-forEachField(R, R->fields(), OneFieldPerUnion, Fn);
-} else {
-  Fn(F);
-}
-
-if (OneFieldPerUnion && Record->isUnion())
-  break;
-  }
-}
-
 } // anonymous namespace
 
 ProTypeMemberInitCheck::ProTypeMemberInitCheck(StringRef Name,
@@ -314,8 +306,14 @@
   if (IsUnion && ClassDecl->hasInClassInitializer())
 return;
 
+  // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet FieldsToInit;
-  fieldsRequiringInit(ClassDecl->fields(), Context, FieldsToInit);
+  forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
+if (!F->hasInClassInitializer() &&
+utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
+Context))
+  FieldsToInit.insert(F);
+  });
   if (FieldsToInit.empty())
 return;
 
@@ -325,7

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-06 Thread Haojian Wu via cfe-commits 
hokein added inline comments.


Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:31
@@ +30,3 @@
+// Convenience utility to get a RecordDecl from a QualType.
+const RecordDecl *getCanonicalRecordDecl(const QualType ) {
+  if (const auto *RT = Type.getCanonicalType()->getAs())

michael_miller wrote:
> alexfh wrote:
> > Is `getCanonicalType()` important here? Did you try using 
> > `QualType::getAsCXXRecordDecl()`?
> Probably not but I didn't try it. I just moved the previous function up to 
> the top so I didn't have to forward declare it.
> 
> One thing I'm simply unsure of is whether it's possible to get a RecordDecl 
> that's not a CXXRecordDecl in C++ code. It seems like the answer is no but 
> that might be another reason to keep it as is if I'm wrong about that...
> One thing I'm simply unsure of is whether it's possible to get a RecordDecl 
> that's not a CXXRecordDecl in C++ code.

From the documents, `RecordDecl` and `CXXRecordDecl` present  
struct/union/class, except `CXXRecordDecl` provides more methods for classes. 
So I think you can try `QualType::getAsCXXRecordDecl()` here.



http://reviews.llvm.org/D19993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-05 Thread Richard Smith via cfe-commits 
On Thu, May 5, 2016 at 4:44 PM, Michael Miller via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> michael_miller added inline comments.
>
> 
> Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:31
> @@ +30,3 @@
> +// Convenience utility to get a RecordDecl from a QualType.
> +const RecordDecl *getCanonicalRecordDecl(const QualType ) {
> +  if (const auto *RT = Type.getCanonicalType()->getAs())
> 
> alexfh wrote:
> > Is `getCanonicalType()` important here? Did you try using
> `QualType::getAsCXXRecordDecl()`?
> Probably not but I didn't try it. I just moved the previous function up to
> the top so I didn't have to forward declare it.
>
> One thing I'm simply unsure of is whether it's possible to get a
> RecordDecl that's not a CXXRecordDecl in C++ code. It seems like the answer
> is no but that might be another reason to keep it as is if I'm wrong about
> that...


It's fine to assume that all RecordDecls will be CXXRecordDecls in C++
code.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-05 Thread Michael Miller via cfe-commits 
michael_miller added inline comments.


Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:31
@@ +30,3 @@
+// Convenience utility to get a RecordDecl from a QualType.
+const RecordDecl *getCanonicalRecordDecl(const QualType ) {
+  if (const auto *RT = Type.getCanonicalType()->getAs())

alexfh wrote:
> Is `getCanonicalType()` important here? Did you try using 
> `QualType::getAsCXXRecordDecl()`?
Probably not but I didn't try it. I just moved the previous function up to the 
top so I didn't have to forward declare it.

One thing I'm simply unsure of is whether it's possible to get a RecordDecl 
that's not a CXXRecordDecl in C++ code. It seems like the answer is no but that 
might be another reason to keep it as is if I'm wrong about that...


http://reviews.llvm.org/D19993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-05 Thread Alexander Kornienko via cfe-commits 
alexfh added inline comments.


Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:31
@@ +30,3 @@
+// Convenience utility to get a RecordDecl from a QualType.
+const RecordDecl *getCanonicalRecordDecl(const QualType ) {
+  if (const auto *RT = Type.getCanonicalType()->getAs())

Is `getCanonicalType()` important here? Did you try using 
`QualType::getAsCXXRecordDecl()`?


http://reviews.llvm.org/D19993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19993: Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

2016-05-05 Thread Michael Miller via cfe-commits 
michael_miller created this revision.
michael_miller added reviewers: hokein, aaron.ballman, alexfh.
michael_miller added a subscriber: cfe-commits.

Fixed a crash in cppcoreguidelines-pro-type-member-init when checking record 
types with indirect fields pre-C++11.
Fixed handling of indirect fields so they are properly checked and suggested 
fixes are proposed.

http://reviews.llvm.org/D19993

Files:
  clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
  test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

Index: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
===
--- test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
+++ test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
@@ -357,3 +357,13 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType
   // CHECK-FIXES: PositiveSelfInitialization() : NegativeAggregateType(), PositiveSelfInitialization() {}
 };
+
+class PositiveIndirectMember {
+  struct {
+int *A;
+// CHECK-FIXES: int *A{};
+  };
+
+  PositiveIndirectMember() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+};
Index: test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
===
--- test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
+++ test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
@@ -103,3 +103,14 @@
 
   int X;
 };
+
+class PositiveIndirectMember {
+  struct {
+int *A;
+  };
+
+  PositiveIndirectMember() : A() {}
+  PositiveIndirectMember(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+  // CHECK-FIXES: PositiveIndirectMember(int) : A() {}
+};
Index: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -27,16 +27,30 @@
 
 namespace {
 
-void fieldsRequiringInit(const RecordDecl::field_range ,
- ASTContext ,
- SmallPtrSetImpl ) {
+// Convenience utility to get a RecordDecl from a QualType.
+const RecordDecl *getCanonicalRecordDecl(const QualType ) {
+  if (const auto *RT = Type.getCanonicalType()->getAs())
+return RT->getDecl();
+  return nullptr;
+}
+
+// Iterate over all the fields in a record type, both direct and indirect (e.g.
+// if the record contains an anonmyous struct). If OneFieldPerUnion is true and
+// the record type (or indirect field) is a union, forEachField will stop after
+// the first field.
+template 
+void forEachField(const RecordDecl *Record, const T ,
+  bool OneFieldPerUnion, Func &) {
   for (const FieldDecl *F : Fields) {
-if (F->hasInClassInitializer())
-  continue;
-QualType Type = F->getType();
-if (!F->hasInClassInitializer() &&
-utils::type_traits::isTriviallyDefaultConstructible(Type, Context))
-  FieldsToInit.insert(F);
+if (F->isAnonymousStructOrUnion()) {
+  if (const RecordDecl *R = getCanonicalRecordDecl(F->getType()))
+forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+} else {
+  Fn(F);
+}
+
+if (OneFieldPerUnion && Record->isUnion())
+  break;
   }
 }
 
@@ -155,13 +169,6 @@
   SmallVector Initializers;
 };
 
-// Convenience utility to get a RecordDecl from a QualType.
-const RecordDecl *getCanonicalRecordDecl(const QualType ) {
-  if (const auto *RT = Type.getCanonicalType()->getAs())
-return RT->getDecl();
-  return nullptr;
-}
-
 template 
 SmallVector
 computeInsertions(const CXXConstructorDecl::init_const_range ,
@@ -179,8 +186,8 @@
   // Gets either the field or base class being initialized by the provided
   // initializer.
   const auto *InitDecl =
-  Init->isMemberInitializer()
-  ? static_cast(Init->getMember())
+  Init->isAnyMemberInitializer()
+  ? static_cast(Init->getAnyMember())
   : Init->getBaseClass()->getAsCXXRecordDecl();
 
   // Add all fields between current field up until the next intializer.
@@ -216,7 +223,8 @@
   Decls.emplace_back(Decl);
 }
   }
-  Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end());
+  forEachField(ClassDecl, ClassDecl->fields(), false,
+   [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template 
@@ -238,22 +246,6 @@
   }
 }
 
-template 
-void forEachField(const RecordDecl *Record, const T ,
-  bool OneFieldPerUnion, Func &) {
-  for (const FieldDecl *F : Fields) {
-if (F->isAnonymousStructOrUnion()) {
-  if (const RecordDecl *R =