njames93 created this revision.
njames93 added reviewers: LegalizeAdulthood, aaron.ballman, alexfh.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, xazax.hun, nemanjai.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Fixes https://github.com/llvm/llvm-project/issues/53515.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D118927
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp
@@ -534,6 +534,24 @@
}
};
+struct PR53515 {
+ int M;
+ // CHECK-MESSAGES: :[[@LINE+5]]:5: warning: 'M' should be initialized in a member initializer of the constructor
+ // CHECK-FIXES: PR53515(const PR53515 &Other) : M(Other.M) {
+ // CHECK-FIXES-NEXT: {{^ $}}
+ // CHECK-FIXES-NEXT: }
+ PR53515(const PR53515 &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: PR53515(PR53515 &&Other) : M(Other.M) {
+ // CHECK-FIXES-NEXT: {{^ $}}
+ // CHECK-FIXES-NEXT: }
+ PR53515(PR53515 &&Other) : M() {
+ M = Other.M;
+ }
+};
+
#define ASSIGN_IN_MACRO(FIELD, VALUE) FIELD = (VALUE);
struct MacroCantFix {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -105,6 +105,12 @@
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- 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
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
@@ -217,14 +217,25 @@
}
} 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();
@@ -235,30 +246,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<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.
- 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<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.
+ InsertPrefix = FirstToCtorInits ? " : " : ", ";
+ }
}
+ InvalidFix |= InsertPos.isMacroID();
}
- InvalidFix |= InsertPos.isMacroID();
SourceLocation SemiColonEnd;
if (auto NextToken = Lexer::findNextToken(
@@ -272,18 +291,22 @@
" 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 ? "), " : ")"});
- Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
- FirstToCtorInits)
- << FixItHint::CreateRemoval(StmtRange);
+ 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);
+ }
+ Diag << FixItHint::CreateRemoval(
+ CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
FirstToCtorInits = false;
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
