[clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

[njames93]

Summary

Fixes https://github.com/llvm/llvm-project/issues/53515.

[simon.giesecke]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

537

Can this be renamed to something describing the test case? E.g. AlreadyHasInitializer (and the case above might be renamed to AlreadyHasDefaultInitializer to differentiate the two cases)

[JonasToth]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

550

could you please add a test-case for initializer lists, like std::vector? Or is this not supported?

[njames93]

Rename test case

[njames93]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

550

That's not what this fix is about.

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp

252

I'm not a fan of using bit-wise operators on booleans, but I see that
the code for this check was already doing that.
(See https://github.com/llvm/llvm-project/issues/40307)

293–308

I'm not a fan of double negatives either :)

[LegalizeAdulthood]

LGTM.

[njames93]

Remove double negative

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp

if (Field) {
                 S->getEndLoc(), *Result.SourceManager, getLangOpts()))
           SemiColonEnd = NextToken->getEndLoc();
         else
           InvalidFix = true;
         auto Diag =
             diag(S->getBeginLoc(), "%0 should be initialized in an in-class"
                                    " default member initializer")
             << Field;
-        if (!InvalidFix) {
-          CharSourceRange StmtRange =
-              CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
+        if (InvalidFix)
+          continue;
 
-          SmallString<128> Insertion(
-              {UseAssignment ? " = " : "{",
-               Lexer::getSourceText(
-                   CharSourceRange(InitValue->getSourceRange(), true),
-                   *Result.SourceManager, getLangOpts()),
-               UseAssignment ? "" : "}"});
 
-          Diag << FixItHint::CreateInsertion(FieldEnd, Insertion)
-               << FixItHint::CreateRemoval(StmtRange);
-        }
+
       } else {
         StringRef InsertPrefix = "";
+        bool HasInitAlready = false;
         SourceLocation InsertPos;
+        SourceRange ReplaceRange;
         bool AddComma = false;
         bool InvalidFix = false;
         unsigned Index = Field->getFieldIndex();
         const CXXCtorInitializer *LastInListInit = nullptr;
         for (const CXXCtorInitializer *Init : Ctor->inits()) {
-          if (!Init->isWritten())
+          if (!Init->isWritten() || Init->isInClassMemberInitializer())
             continue;
+          if (Init->getMember() == Field) {
+            HasInitAlready = true;
+            if (isa<ImplicitValueInitExpr>(Init->getInit()))
+              InsertPos = Init->getRParenLoc();
+            else {
+              ReplaceRange = Init->getInit()->getSourceRange();
+            }
+            break;
+          }
           if (Init->isMemberInitializer() &&
               Index < Init->getMember()->getFieldIndex()) {
             InsertPos = Init->getSourceLocation();
             // There are initializers after the one we are inserting, so add a
             // comma after this insertion in order to not break anything.
             AddComma = true;
             break;
           }
           LastInListInit = Init;
         }
+        if (HasInitAlready) {
+          if (InsertPos.isValid())
+            InvalidFix |= InsertPos.isMacroID();
+          else
+            InvalidFix |= ReplaceRange.getBegin().isMacroID() ||
+                          ReplaceRange.getEnd().isMacroID();
+        } else {
           if (InsertPos.isInvalid()) {
             if (LastInListInit) {
               InsertPos = Lexer::getLocForEndOfToken(
                   LastInListInit->getRParenLoc(), 0, *Result.SourceManager,
                   getLangOpts());
               // Inserting after the last constructor initializer, so we need a
               // comma.
               InsertPrefix = ", ";
             } else {
               InsertPos = Lexer::getLocForEndOfToken(
                   Ctor->getTypeSourceInfo()
                       ->getTypeLoc()
                       .getAs<clang::FunctionTypeLoc>()
                       .getLocalRangeEnd(),
                   0, *Result.SourceManager, getLangOpts());
 
               // If this is first time in the loop, there are no initializers so
-            // `:` declares member initialization list. If this is a subsequent
-            // pass then we have already inserted a `:` so continue with a
-            // comma.
+              // `:` declares member initialization list. If this is a
+              // subsequent pass then we have already inserted a `:` so continue
+              // with a comma.
               InsertPrefix = FirstToCtorInits ? " : " : ", ";
             }
           }
           InvalidFix |= InsertPos.isMacroID();
+        }
 
         SourceLocation SemiColonEnd;
         if (auto NextToken = Lexer::findNextToken(
                 S->getEndLoc(), *Result.SourceManager, getLangOpts()))
           SemiColonEnd = NextToken->getEndLoc();
         else
           InvalidFix = true;
 
         auto Diag =
             diag(S->getBeginLoc(), "%0 should be initialized in a member"
                                    " initializer of the constructor")
             << Field;
-        if (!InvalidFix) {
-
-          CharSourceRange StmtRange =
-              CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
-          SmallString<128> Insertion(
-              {InsertPrefix, Field->getName(), "(",
-               Lexer::getSourceText(
-                   CharSourceRange(InitValue->getSourceRange(), true),
-                   *Result.SourceManager, getLangOpts()),
-               AddComma ? "), " : ")"});
+        if (InvalidFix)
+          continue;
+        StringRef NewInit = Lexer::getSourceText(
+            *Result.SourceManager, getLangOpts());
+        if (HasInitAlready) {
+          if (InsertPos.isValid())
+            Diag << FixItHint::CreateInsertion(InsertPos, NewInit);
+          else
+            Diag << FixItHint::CreateReplacement(ReplaceRange, NewInit);
+        } else {
+          SmallString<128> Insertion({InsertPrefix, Field->getName(), "(",
+                                      NewInit, AddComma ? "), " : ")"});
           Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
-                                             FirstToCtorInits)
-               << FixItHint::CreateRemoval(StmtRange);
-          FirstToCtorInits = false;
+                                             FirstToCtorInits);
         }
+        Diag << FixItHint::CreateRemoval(
+            CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
+        FirstToCtorInits = false;
       }
     }
   }
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed a crash in :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` when a pure virtual function
   overrided has a const return type. Removed the fix for a virtual function.
 
 - Fixed a false positive in :doc:`misc-redundant-expression <clang-tidy/checks/misc-redundant-expression>`
   involving overloaded comparison operators.
 
 - Fixed a crash in :doc:`bugprone-sizeof-expression <clang-tidy/checks/bugprone-sizeof-expression>` when
   `sizeof(...)` is compared agains a `__int128_t`.
   
+- Improved :doc:`cppcoreguidelines-prefer-member-initializer
+  <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
+
+  Fixed an issue when there was already an initializer in the constructor and
+  the check would try to create another initializer for the same member.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

struct InitFromVarDecl {
   int m;
   InitFromVarDecl() {
     // Can't apply this fix as n is declared in the body of the constructor.
     int n = 3;
     m = n;
   }
 };
 
-struct AlreadyHasInit {
+struct HasInClassInit {
   int m = 4;
-  AlreadyHasInit() {
+  HasInClassInit() {
     m = 3;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'm' should be initialized in a member initializer of the constructor
   }
 };
 
+struct HasInitListInit {
+  int M;
+  // CHECK-MESSAGES: :[[@LINE+5]]:5: warning: 'M' should be initialized in a member initializer of the constructor
+  // CHECK-FIXES: HasInitListInit(const HasInitListInit &Other) : M(Other.M) {
+  // CHECK-FIXES-NEXT: {{^    $}}
+  // CHECK-FIXES-NEXT: }
+  HasInitListInit(const HasInitListInit &Other) : M(4) {
+    M = Other.M;
+  }
+  // CHECK-MESSAGES: :[[@LINE+5]]:5: warning: 'M' should be initialized in a member initializer of the constructor
+  // CHECK-FIXES: HasInitListInit(HasInitListInit &&Other) : M(Other.M) {
+  // CHECK-FIXES-NEXT: {{^    $}}
+  // CHECK-FIXES-NEXT: }
+  HasInitListInit(HasInitListInit &&Other) : M() {
+    M = Other.M;
+  }
+};
+
 #define ASSIGN_IN_MACRO(FIELD, VALUE) FIELD = (VALUE);
 
 struct MacroCantFix {
   int n; // NoFix
   // CHECK-FIXES: int n; // NoFix
   MacroCantFix() {
     ASSIGN_IN_MACRO(n, 0)
     // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'n' should be initialized in a member initializer of the constructor
     // CHECK-FIXES: ASSIGN_IN_MACRO(n, 0)
   }
 };