njames93 updated this revision to Diff 325482.
njames93 added a comment.
Add a test for the crash fix.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97132/new/
https://reviews.llvm.org/D97132
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
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
@@ -488,3 +488,60 @@
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'x' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
// CHECK-FIXES: {{^\ *$}}
}
+
+struct SafeDependancy {
+ int m;
+ int n;
+ SafeDependancy(int M) : m(M) {
+ // CHECK-FIXES: SafeDependancy(int M) : m(M), n(m) {
+ n = m;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in a member initializer of the constructor
+ }
+ // We match against direct field dependancy as well as descendant field
+ // dependancy, ensure both are accounted for.
+ SafeDependancy(short M) : m(M) {
+ // CHECK-FIXES: SafeDependancy(short M) : m(M), n(m + 1) {
+ n = m + 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in a member initializer of the constructor
+ }
+};
+
+struct BadDependancy {
+ int m;
+ int n;
+ BadDependancy(int N) : n(N) {
+ m = n;
+ }
+ BadDependancy(short N) : n(N) {
+ m = n + 1;
+ }
+};
+
+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 {
+ int m = 4;
+ AlreadyHasInit() {
+ m = 3;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'm' should be initialized in a member initializer of the constructor
+ }
+};
+
+#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)
+ }
+};
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
@@ -55,8 +55,37 @@
return false;
}
+namespace {
+AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) {
+ return Node.getFieldIndex() >= Index;
+}
+} // namespace
+
+// Checks if Field is initialised using a field that will be initialised after
+// it.
+// TODO: Probably should guard against function calls that could have side
+// effects or if they do reference another field that's initialized before this
+// field, but is modified before the assignment.
+static bool isSafeAssignment(const FieldDecl *Field, const Expr *Init,
+ const CXXConstructorDecl *Context) {
+
+ auto MemberMatcher =
+ memberExpr(hasObjectExpression(cxxThisExpr()),
+ member(fieldDecl(indexNotLessThan(Field->getFieldIndex()))));
+
+ auto DeclMatcher = declRefExpr(
+ to(varDecl(unless(parmVarDecl()), hasDeclContext(equalsNode(Context)))));
+
+ return match(expr(anyOf(MemberMatcher, DeclMatcher,
+ hasDescendant(MemberMatcher),
+ hasDescendant(DeclMatcher))),
+ *Init, Field->getASTContext())
+ .empty();
+}
+
static const std::pair<const FieldDecl *, const Expr *>
-isAssignmentToMemberOf(const RecordDecl *Rec, const Stmt *S) {
+isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S,
+ const CXXConstructorDecl *Ctor) {
if (const auto *BO = dyn_cast<BinaryOperator>(S)) {
if (BO->getOpcode() != BO_Assign)
return std::make_pair(nullptr, nullptr);
@@ -69,8 +98,11 @@
if (!Field)
return std::make_pair(nullptr, nullptr);
- if (isa<CXXThisExpr>(ME->getBase()))
- return std::make_pair(Field, BO->getRHS()->IgnoreParenImpCasts());
+ if (!isa<CXXThisExpr>(ME->getBase()))
+ return std::make_pair(nullptr, nullptr);
+ const auto *Init = BO->getRHS()->IgnoreParenImpCasts();
+ if (isSafeAssignment(Field, Init, Ctor))
+ return std::make_pair(Field, Init);
} else if (const auto *COCE = dyn_cast<CXXOperatorCallExpr>(S)) {
if (COCE->getOperator() != OO_Equal)
return std::make_pair(nullptr, nullptr);
@@ -84,8 +116,11 @@
if (!Field)
return std::make_pair(nullptr, nullptr);
- if (isa<CXXThisExpr>(ME->getBase()))
- return std::make_pair(Field, COCE->getArg(1)->IgnoreParenImpCasts());
+ if (!isa<CXXThisExpr>(ME->getBase()))
+ return std::make_pair(nullptr, nullptr);
+ const auto *Init = COCE->getArg(1)->IgnoreParenImpCasts();
+ if (isSafeAssignment(Field, Init, Ctor))
+ return std::make_pair(Field, Init);
}
return std::make_pair(nullptr, nullptr);
@@ -118,14 +153,12 @@
const auto *Body = cast<CompoundStmt>(Ctor->getBody());
const CXXRecordDecl *Class = Ctor->getParent();
- SourceLocation InsertPos;
bool FirstToCtorInits = true;
for (const Stmt *S : Body->body()) {
if (S->getBeginLoc().isMacroID()) {
- StringRef MacroName =
- Lexer::getImmediateMacroName(S->getBeginLoc(), *Result.SourceManager,
- getLangOpts());
+ StringRef MacroName = Lexer::getImmediateMacroName(
+ S->getBeginLoc(), *Result.SourceManager, getLangOpts());
if (MacroName.contains_lower("assert"))
return;
}
@@ -143,97 +176,116 @@
const FieldDecl *Field;
const Expr *InitValue;
- std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S);
+ std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S, Ctor);
if (Field) {
if (IsUseDefaultMemberInitEnabled && getLangOpts().CPlusPlus11 &&
Ctor->isDefaultConstructor() &&
(getLangOpts().CPlusPlus20 || !Field->isBitField()) &&
+ !Field->hasInClassInitializer() &&
(!isa<RecordDecl>(Class->getDeclContext()) ||
!cast<RecordDecl>(Class->getDeclContext())->isUnion()) &&
shouldBeDefaultMemberInitializer(InitValue)) {
+ bool InvalidFix = false;
SourceLocation FieldEnd =
Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
*Result.SourceManager, getLangOpts());
- SmallString<128> Insertion(
- {UseAssignment ? " = " : "{",
- Lexer::getSourceText(
- CharSourceRange(InitValue->getSourceRange(), true),
- *Result.SourceManager, getLangOpts()),
- UseAssignment ? "" : "}"});
-
- SourceLocation SemiColonEnd =
- Lexer::findNextToken(S->getEndLoc(), *Result.SourceManager,
- getLangOpts())
- ->getEndLoc();
- CharSourceRange StmtRange =
- CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
-
- diag(S->getBeginLoc(), "%0 should be initialized in an in-class"
- " default member initializer")
- << Field << FixItHint::CreateInsertion(FieldEnd, Insertion)
- << FixItHint::CreateRemoval(StmtRange);
+ InvalidFix |= FieldEnd.isInvalid() || FieldEnd.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 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);
+ }
} else {
- SmallString<128> Insertion;
+ StringRef InsertPrefix = "";
+ SourceLocation InsertPos;
bool AddComma = false;
- if (!Ctor->getNumCtorInitializers() && FirstToCtorInits) {
- SourceLocation BodyPos = Ctor->getBody()->getBeginLoc();
- SourceLocation NextPos = Ctor->getBeginLoc();
- do {
- InsertPos = NextPos;
- NextPos = Lexer::findNextToken(NextPos, *Result.SourceManager,
- getLangOpts())
- ->getLocation();
- } while (NextPos != BodyPos);
- InsertPos = Lexer::getLocForEndOfToken(
- InsertPos, 0, *Result.SourceManager, getLangOpts());
-
- Insertion = " : ";
- } else {
- bool Found = false;
- unsigned Index = Field->getFieldIndex();
- for (const auto *Init : Ctor->inits()) {
- if (Init->isMemberInitializer()) {
- if (Index < Init->getMember()->getFieldIndex()) {
- InsertPos = Init->getSourceLocation();
- Found = true;
- break;
- }
- }
+ bool InvalidFix = false;
+ unsigned Index = Field->getFieldIndex();
+ const CXXCtorInitializer *LastInListInit = nullptr;
+ for (const CXXCtorInitializer *Init : Ctor->inits()) {
+ if (!Init->isWritten())
+ continue;
+ 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;
}
-
- if (!Found) {
- if (Ctor->getNumCtorInitializers()) {
- InsertPos = Lexer::getLocForEndOfToken(
- (*Ctor->init_rbegin())->getSourceRange().getEnd(), 0,
- *Result.SourceManager, getLangOpts());
- }
- Insertion = ", ";
+ 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 {
- AddComma = true;
+ 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 ? " : " : ", ";
}
}
- Insertion.append(
- {Field->getName(), "(",
- Lexer::getSourceText(
- CharSourceRange(InitValue->getSourceRange(), true),
- *Result.SourceManager, getLangOpts()),
- AddComma ? "), " : ")"});
-
- SourceLocation SemiColonEnd =
- Lexer::findNextToken(S->getEndLoc(), *Result.SourceManager,
- getLangOpts())
- ->getEndLoc();
- CharSourceRange StmtRange =
- CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
-
- diag(S->getBeginLoc(), "%0 should be initialized in a member"
- " initializer of the constructor")
- << Field
- << FixItHint::CreateInsertion(InsertPos, Insertion,
- FirstToCtorInits)
- << FixItHint::CreateRemoval(StmtRange);
- FirstToCtorInits = false;
+ 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 ? "), " : ")"});
+ Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
+ FirstToCtorInits)
+ << FixItHint::CreateRemoval(StmtRange);
+ FirstToCtorInits = false;
+ }
}
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
