[clang-tools-extra] d0fcbb3 - [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 7 11:14:03 PDT 2022
Author: Nathan James
Date: 2022-04-07T19:13:50+01:00
New Revision: d0fcbb37838a653c1c3f8d6ac83e64714c407b19
URL: https://github.com/llvm/llvm-project/commit/d0fcbb37838a653c1c3f8d6ac83e64714c407b19
DIFF: https://github.com/llvm/llvm-project/commit/d0fcbb37838a653c1c3f8d6ac83e64714c407b19.diff
LOG: [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer
Fixes https://github.com/llvm/llvm-project/issues/53515.
Reviewed By: LegalizeAdulthood
Differential Revision: https://reviews.llvm.org/D118927
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
index 169b828a3c926..4ff362476a0c3 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
@@ -201,30 +201,42 @@ void PreferMemberInitializerCheck::check(
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<ImplicitValueInitExpr>(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 @@ void PreferMemberInitializerCheck::check(
}
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(
@@ -271,21 +291,25 @@ void PreferMemberInitializerCheck::check(
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
index 919ec64519232..58ddb116f9840 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -139,6 +139,12 @@ Changes in existing checks
- 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
^^^^^^^^^^^^^^
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
index 73eeb2139b6ab..e1929c2a3f055 100644
--- 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 InitFromVarDecl {
}
};
-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 {
More information about the cfe-commits
mailing list