diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp @@ -201,30 +201,42 @@ diag(S->getBeginLoc(), "%0 should be initialized in an in-class" " default member initializer") << Field; - if (!InvalidFix) { - CharSourceRange StmtRange = - CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd); - - SmallString<128> Insertion( - {UseAssignment ? " = " : "{", - Lexer::getSourceText( - CharSourceRange(InitValue->getSourceRange(), true), - *Result.SourceManager, getLangOpts()), - UseAssignment ? "" : "}"}); - - Diag << FixItHint::CreateInsertion(FieldEnd, Insertion) - << FixItHint::CreateRemoval(StmtRange); - } + if (InvalidFix) + continue; + CharSourceRange StmtRange = + CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd); + + 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(Init->getInit())) + InsertPos = Init->getRParenLoc(); + else { + ReplaceRange = Init->getInit()->getSourceRange(); + } + break; + } if (Init->isMemberInitializer() && Index < Init->getMember()->getFieldIndex()) { InsertPos = Init->getSourceLocation(); @@ -235,30 +247,38 @@ } LastInListInit = Init; } - 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() - .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. - InsertPrefix = FirstToCtorInits ? " : " : ", "; + 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() + .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. + InsertPrefix = FirstToCtorInits ? " : " : ", "; + } } + InvalidFix |= InsertPos.isMacroID(); } - InvalidFix |= InsertPos.isMacroID(); SourceLocation SemiColonEnd; if (auto NextToken = Lexer::findNextToken( @@ -271,21 +291,25 @@ 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( + CharSourceRange(InitValue->getSourceRange(), true), + *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; } } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -109,6 +109,12 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`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 ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp @@ -526,14 +526,32 @@ } }; -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 {