[clang-tools-extra] r312166 - [cppcoreguidelines] Don't rely on SmallPtrSet iteration order.

[Benjamin Kramer via cfe-commits]

Author: d0k
Date: Wed Aug 30 13:18:40 2017
New Revision: 312166

URL: http://llvm.org/viewvc/llvm-project?rev=312166&view=rev
Log:
[cppcoreguidelines] Don't rely on SmallPtrSet iteration order.

The fixit emission breaks if the iteration order changes and also missed
to emit fixits for some edge cases.

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

Modified: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp?rev=312166&r1=312165&r2=312166&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp Wed Aug 30 13:18:40 2017
@@ -32,22 +32,16 @@ AST_MATCHER(CXXRecordDecl, hasDefaultCon
 }
 
 // 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.
+// if the record contains an anonmyous struct).
 template <typename T, typename Func>
-void forEachField(const RecordDecl &Record, const T &Fields,
-                  bool OneFieldPerUnion, Func &&Fn) {
+void forEachField(const RecordDecl &Record, const T &Fields, Func &&Fn) {
   for (const FieldDecl *F : Fields) {
     if (F->isAnonymousStructOrUnion()) {
       if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
-        forEachField(*R, R->fields(), OneFieldPerUnion, Fn);
+        forEachField(*R, R->fields(), Fn);
     } else {
       Fn(F);
     }
-
-    if (OneFieldPerUnion && Record.isUnion())
-      break;
   }
 }
 
@@ -227,7 +221,7 @@ void getInitializationsInOrder(const CXX
       Decls.emplace_back(Decl);
     }
   }
-  forEachField(ClassDecl, ClassDecl.fields(), false,
+  forEachField(ClassDecl, ClassDecl.fields(),
                [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
@@ -353,7 +347,7 @@ void ProTypeMemberInitCheck::checkMissin
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
-  forEachField(ClassDecl, ClassDecl.fields(), false, [&](const FieldDecl *F) {
+  forEachField(ClassDecl, ClassDecl.fields(), [&](const FieldDecl *F) {
     if (!F->hasInClassInitializer() &&
         utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
                                                             Context) &&
@@ -379,12 +373,12 @@ void ProTypeMemberInitCheck::checkMissin
   // Collect all fields in order, both direct fields and indirect fields from
   // anonmyous record types.
   SmallVector<const FieldDecl *, 16> OrderedFields;
-  forEachField(ClassDecl, ClassDecl.fields(), false,
+  forEachField(ClassDecl, ClassDecl.fields(),
                [&](const FieldDecl *F) { OrderedFields.push_back(F); });
 
   // Collect all the fields we need to initialize, including indirect fields.
   SmallPtrSet<const FieldDecl *, 16> AllFieldsToInit;
-  forEachField(ClassDecl, FieldsToInit, false,
+  forEachField(ClassDecl, FieldsToInit,
                [&](const FieldDecl *F) { AllFieldsToInit.insert(F); });
   if (AllFieldsToInit.empty())
     return;
@@ -404,11 +398,16 @@ void ProTypeMemberInitCheck::checkMissin
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet<const FieldDecl *, 16> FieldsToFix;
-  forEachField(ClassDecl, FieldsToInit, true, [&](const FieldDecl *F) {
+  SmallPtrSet<const RecordDecl *, 4> UnionsSeen;
+  forEachField(ClassDecl, OrderedFields, [&](const FieldDecl *F) {
+    if (!FieldsToInit.count(F))
+      return;
     // Don't suggest fixes for enums because we don't know a good default.
     // Don't suggest fixes for bitfields because in-class initialization is not
     // possible.
-    if (!F->getType()->isEnumeralType() && !F->isBitField())
+    if (F->getType()->isEnumeralType() || F->isBitField())
+      return;
+    if (!F->getParent()->isUnion() || UnionsSeen.insert(F->getParent()).second)
       FieldsToFix.insert(F);
   });
   if (FieldsToFix.empty())

Modified: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp?rev=312166&r1=312165&r2=312166&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp Wed Aug 30 13:18:40 2017
@@ -312,6 +312,19 @@ union PositiveUnion {
   // CHECK-FIXES-NOT: float Y{};
 };
 
+union PositiveUnionReversed {
+  PositiveUnionReversed() : X() {} // No message as a union can only initialize one member.
+  PositiveUnionReversed(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: Y, X
+
+  // Make sure we don't give Y an initializer.
+  TestEnum Y;
+  // CHECK-FIXES-NOT: TestEnum Y{};
+
+  int X;
+  // CHECK-FIXES: int X{};
+};
+
 struct PositiveAnonymousUnionAndStruct {
   PositiveAnonymousUnionAndStruct() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B, Y, Z, C, D, E, F, X