Author: Congcong Cai Date: 2023-10-27T23:56:25+08:00 New Revision: 263746754ab2ad099ffb22112cda5926d7ae3353
URL: https://github.com/llvm/llvm-project/commit/263746754ab2ad099ffb22112cda5926d7ae3353 DIFF: https://github.com/llvm/llvm-project/commit/263746754ab2ad099ffb22112cda5926d7ae3353.diff LOG: [clang-tidy]Fix PreferMemberInitializer false positive for reassignment (#70316) - assignment twice cannot be simplified to once when assigning to reference type - safe assignment cannot be advanced before unsafe assignment 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 0ef13ae29803325..b6daf8b936bde0f 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp @@ -8,8 +8,10 @@ #include "PreferMemberInitializerCheck.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" +#include "llvm/ADT/DenseMap.h" using namespace clang::ast_matchers; @@ -54,31 +56,66 @@ static bool shouldBeDefaultMemberInitializer(const Expr *Value) { } namespace { + AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) { return Node.getFieldIndex() >= Index; } + +enum class AssignedLevel { + // Field is not assigned. + None, + // Field is assigned. + Default, + // Assignment of field has side effect: + // - assign to reference. + // FIXME: support other side effect. + HasSideEffect, + // Assignment of field has data dependence. + HasDependence, +}; + } // namespace +static bool canAdvanceAssignment(AssignedLevel Level) { + return Level == AssignedLevel::None || Level == AssignedLevel::Default; +} + // 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) { +// effects or if they do reference another field that's initialized before +// this field, but is modified before the assignment. +static void updateAssignmentLevel( + const FieldDecl *Field, const Expr *Init, const CXXConstructorDecl *Ctor, + llvm::DenseMap<const FieldDecl *, AssignedLevel> &AssignedFields) { + auto It = AssignedFields.find(Field); + if (It == AssignedFields.end()) + It = AssignedFields.insert({Field, AssignedLevel::None}).first; + + if (!canAdvanceAssignment(It->second)) + // fast path for already decided field. + return; + + if (Field->getType().getCanonicalType()->isReferenceType()) { + // assign to reference type twice cannot be simplified to once. + It->second = AssignedLevel::HasSideEffect; + return; + } 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(); + to(varDecl(unless(parmVarDecl()), hasDeclContext(equalsNode(Ctor))))); + const bool HasDependence = !match(expr(anyOf(MemberMatcher, DeclMatcher, + hasDescendant(MemberMatcher), + hasDescendant(DeclMatcher))), + *Init, Field->getASTContext()) + .empty(); + if (HasDependence) { + It->second = AssignedLevel::HasDependence; + return; + } } static std::pair<const FieldDecl *, const Expr *> @@ -99,9 +136,9 @@ isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S, if (!isa<CXXThisExpr>(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<CXXOperatorCallExpr>(S)) { + return std::make_pair(Field, Init); + } + if (const auto *COCE = dyn_cast<CXXOperatorCallExpr>(S)) { if (COCE->getOperator() != OO_Equal) return std::make_pair(nullptr, nullptr); @@ -117,10 +154,8 @@ isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S, if (!isa<CXXThisExpr>(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(Field, Init); } - return std::make_pair(nullptr, nullptr); } @@ -156,6 +191,12 @@ void PreferMemberInitializerCheck::check( const CXXRecordDecl *Class = Ctor->getParent(); bool FirstToCtorInits = true; + llvm::DenseMap<const FieldDecl *, AssignedLevel> AssignedFields{}; + + for (const CXXCtorInitializer *Init : Ctor->inits()) + if (FieldDecl *Field = Init->getMember()) + updateAssignmentLevel(Field, Init->getInit(), Ctor, AssignedFields); + for (const Stmt *S : Body->body()) { if (S->getBeginLoc().isMacroID()) { StringRef MacroName = Lexer::getImmediateMacroName( @@ -180,6 +221,9 @@ void PreferMemberInitializerCheck::check( std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S, Ctor); if (!Field) continue; + updateAssignmentLevel(Field, InitValue, Ctor, AssignedFields); + if (!canAdvanceAssignment(AssignedFields[Field])) + continue; const bool IsInDefaultMemberInitializer = IsUseDefaultMemberInitEnabled && getLangOpts().CPlusPlus11 && Ctor->isDefaultConstructor() && diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a511a522efb2e44..5ce38ab48bf295f 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -256,7 +256,8 @@ Changes in existing checks - Improved :doc:`cppcoreguidelines-prefer-member-initializer <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to - ignore delegate constructors. + ignore delegate constructors and ignore re-assignment for reference or when + initialization depend on field that is initialized before. - Improved :doc:`cppcoreguidelines-pro-bounds-array-to-pointer-decay <clang-tidy/checks/cppcoreguidelines/pro-bounds-array-to-pointer-decay>` check 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 9d7aad52c8fa801..b5603dea316d596 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 @@ -570,3 +570,19 @@ struct PR52818 { int bar; }; + +struct RefReassignment { + RefReassignment(int &i) : m_i{i} { + m_i = 1; + } + int & m_i; +}; + +struct ReassignmentAfterUnsafetyAssignment { + ReassignmentAfterUnsafetyAssignment() { + int a = 10; + m_i = a; + m_i = 1; + } + int m_i; +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
