[PATCH] D18584: Complete support for C++ Core Guidelines Type.6: Always initialize a member variable.

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman requested changes to this revision.
aaron.ballman added a reviewer: aaron.ballman.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:25
@@ -24,3 +24,3 @@
 
-namespace {
 
----------------
Please do not remove the unnamed namespace; it is preferable to using static functions.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:30
@@ +29,3 @@
+  const auto *ClassDecl = dyn_cast<CXXRecordDecl>(&RecordDecl);
+  // Non-C++ records are always trivially constructible
+  if (!ClassDecl)
----------------
Missing a period at the end of the sentence.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:33
@@ +32,3 @@
+    return true;
+  // A class is trivially constructible if it has a default constructor.
+  if (ClassDecl->hasUserProvidedDefaultConstructor())
----------------
This comment doesn't match the behavior of the code below.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:104
@@ -35,3 +103,3 @@
     QualType Type = F->getType();
-    if (Type->isPointerType() || Type->isBuiltinType())
+    if (!F->hasInClassInitializer() and isTriviallyConstructible(Context, Type))
       FieldsToInit.insert(F);
----------------
Please use `&&` instead of `and`.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:152
@@ +151,3 @@
+
+AST_MATCHER(CXXConstructorDecl, isUserProvided) {
+  return Node.isUserProvided();
----------------
This should probably be made more generally available as an AST matcher.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:156
@@ +155,3 @@
+
+AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) {
+  return isTriviallyConstructible(Finder->getASTContext(), Node);
----------------
I think this matcher logic should go into utils\TypeTraits.cpp as it seems more generally useful than just this check.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:160
@@ +159,3 @@
+
+AST_MATCHER(CXXConstructorDecl, isDelegatingConstructor) {
+  return Node.isDelegatingConstructor();
----------------
As should this.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:187
@@ -87,5 +186,3 @@
                              const CXXConstructorDecl &Constructor) const {
-    if (InitializerBefore != nullptr) {
-      return Lexer::getLocForEndOfToken(InitializerBefore->getRParenLoc(), 0,
-                                        Context.getSourceManager(),
-                                        Context.getLangOpts());
+    assert(Where != nullptr or Placement == InitializerPlacement::New);
+    SourceLocation Location;
----------------
Please use `||` instead of `or`. Also, asserts should usually have `&& "Some explanatory text"` for when the assert is triggered.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:243
@@ +242,3 @@
+// initializer.
+static const NamedDecl *getInitializerDecl(const CXXCtorInitializer *Init) {
+  if (Init->isMemberInitializer())
----------------
Since this function is only used once, I think it should be lowered into its use as a ternary conditional.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:256
@@ +255,3 @@
+  SmallVector<IntializerInsertion, 16> Insertions;
+  Insertions.push_back({InitializerPlacement::New, nullptr, {}});
+
----------------
Please do not use an initializer list like this, I believe it won't work in MSVC 2013. (Here and elsewhere as well.)

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:258
@@ -142,1 +257,3 @@
+
+  auto Decl = std::begin(OrderedDecls);
   for (const CXXCtorInitializer *Init : Inits) {
----------------
Please only use `auto` when the type name is spelled out as part of the initializer, or it is used as a for-range initializer.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:296
@@ +295,3 @@
+  Decls.clear();
+  for (const CXXBaseSpecifier &Base : ClassDecl->bases())
+    Decls.emplace_back(getRecordDecl(Base.getType()));
----------------
Can use `const auto &` here instead.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:323
@@ -171,5 +322,3 @@
 void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(cxxConstructorDecl(isDefinition(), isUserProvided(),
-                                        unless(isInstantiated()))
-                         .bind("ctor"),
-                     this);
+  auto IsUserProvidedNonDelegatingConstructor =
+      allOf(isUserProvided(),
----------------
Since this is a C++ core guideline, I think the matchers should only be registered when compiling for C++. It may make sense to use a similar check in C mode, but we can cross that bridge when we come to it (and have tests for it).

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:486
@@ +485,3 @@
+
+  // Do not propose fixes in macros since we cannot place them correctly.
+  if (Ctor->getLocStart().isMacroID())
----------------
I think this code belongs in `fixInitializerList` then, doesn't it?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:497
@@ +496,3 @@
+      diag(Var->getLocStart(), "uninitialized record type: %0")
+      << Var->getName().str();
+
----------------
You should be able to pass in `Var` directly.

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:285
@@ +284,3 @@
+union NegativeUnionInClass {
+  NegativeUnionInClass() {} // No message as a union can only initialize on member
+  int X = 0;
----------------
s/on/one

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:291
@@ +290,3 @@
+union PositiveUnion {
+  PositiveUnion() : X() {} // No message as a union can only initialize on member
+  PositiveUnion(int) {}
----------------
s/on/one

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:334
@@ +333,2 @@
+  // CHECK-FIXES: int X{};
+};
----------------
I'd like to see how we handle the following test:
```
struct S {
  S(int i);
};
struct T {
  T(float f);
};

union U {
  S s;
  T t;
};
```


http://reviews.llvm.org/D18584