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 @@ -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 -isAssignmentToMemberOf(const RecordDecl *Rec, const Stmt *S) { +isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S, + const CXXConstructorDecl *Ctor) { if (const auto *BO = dyn_cast(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(ME->getBase())) - return std::make_pair(Field, BO->getRHS()->IgnoreParenImpCasts()); + if (!isa(ME->getBase())) + return std::make_pair(nullptr, nullptr); + const Expr *Init = BO->getRHS()->IgnoreParenImpCasts(); + if (isSafeAssignment(Field, Init, Ctor)) + return std::make_pair(Field, Init); } else if (const auto *COCE = dyn_cast(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(ME->getBase())) - return std::make_pair(Field, COCE->getArg(1)->IgnoreParenImpCasts()); + if (!isa(ME->getBase())) + return std::make_pair(nullptr, nullptr); + const Expr *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(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(Class->getDeclContext()) || !cast(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() + .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; + } } } } 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 @@ -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) + } +};