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 @@ -8,7 +8,9 @@ #include "PreferMemberInitializerCheck.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/DeclCXX.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -55,8 +57,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 +100,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 auto *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 +118,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 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 +155,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,11 +178,12 @@ 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)) { @@ -175,42 +211,47 @@ << FixItHint::CreateRemoval(StmtRange); } else { SmallString<128> Insertion; + 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; - } - } + unsigned Index = Field->getFieldIndex(); + const CXXCtorInitializer *LastInListInit = nullptr; + for (const CXXCtorInitializer *Init : Ctor->inits()) { + if (Init->isInClassMemberInitializer()) + 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()); - } + 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. Insertion = ", "; } else { - AddComma = true; + 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()); + // 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. + Insertion = FirstToCtorInits ? " : " : ", "; } } Insertion.append( 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,48 @@ // 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 + } +};