https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/70316
>From 41278ca046ae5d4be4ac4ac1bc673b5010668d9c Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Thu, 26 Oct 2023 18:54:05 +0800 Subject: [PATCH 1/2] [clang-tidy]Fix PreferMemberInitializer false positive for reassignment - assignment twice cannot be simplified to once when assigning to reference type - Original design doesn't consider safe assignment cannot be advanced before unsafe assignment --- .../PreferMemberInitializerCheck.cpp | 46 +++++++++++++++---- clang-tools-extra/docs/ReleaseNotes.rst | 3 +- .../prefer-member-initializer.cpp | 16 +++++++ 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp index 0ef13ae29803325..ae91ae22612c40b 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 <map> using namespace clang::ast_matchers; @@ -54,9 +56,13 @@ static bool shouldBeDefaultMemberInitializer(const Expr *Value) { } namespace { + AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) { return Node.getFieldIndex() >= Index; } + +enum class AssignedLevel { None, Assigned, UnsafetyAssigned }; + } // namespace // Checks if Field is initialised using a field that will be initialised after @@ -64,13 +70,19 @@ AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) { // 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) { +static AssignedLevel isSafeAssignment(const FieldDecl *Field, const Expr *Init, + const CXXConstructorDecl *Context, + AssignedLevel HistoryLevel) { + if (HistoryLevel == AssignedLevel::UnsafetyAssigned) + return AssignedLevel::UnsafetyAssigned; + if (Field->getType()->isReferenceType() && + HistoryLevel == AssignedLevel::Assigned) + // assign to reference type twice cannot be simplified to once. + return AssignedLevel::UnsafetyAssigned; auto MemberMatcher = memberExpr(hasObjectExpression(cxxThisExpr()), member(fieldDecl(indexNotLessThan(Field->getFieldIndex())))); - auto DeclMatcher = declRefExpr( to(varDecl(unless(parmVarDecl()), hasDeclContext(equalsNode(Context))))); @@ -78,7 +90,9 @@ static bool isSafeAssignment(const FieldDecl *Field, const Expr *Init, hasDescendant(MemberMatcher), hasDescendant(DeclMatcher))), *Init, Field->getASTContext()) - .empty(); + .empty() + ? AssignedLevel::Assigned + : AssignedLevel::UnsafetyAssigned; } static std::pair<const FieldDecl *, const Expr *> @@ -99,9 +113,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 +131,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 +168,12 @@ void PreferMemberInitializerCheck::check( const CXXRecordDecl *Class = Ctor->getParent(); bool FirstToCtorInits = true; + std::map<const FieldDecl *, AssignedLevel> AssignedFields{}; + + for (const CXXCtorInitializer *Init : Ctor->inits()) + if (FieldDecl *Field = Init->getMember()) + AssignedFields.insert({Field, AssignedLevel::Assigned}); + for (const Stmt *S : Body->body()) { if (S->getBeginLoc().isMacroID()) { StringRef MacroName = Lexer::getImmediateMacroName( @@ -180,6 +198,14 @@ void PreferMemberInitializerCheck::check( std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S, Ctor); if (!Field) continue; + const auto It = AssignedFields.find(Field); + AssignedLevel Level = AssignedLevel::None; + if (It != AssignedFields.end()) + Level = It->second; + Level = isSafeAssignment(Field, InitValue, Ctor, Level); + AssignedFields.insert_or_assign(Field, Level); + if (Level == AssignedLevel::UnsafetyAssigned) + 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 c93775beb8aeaf5..5536f9e805d8f70 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -250,7 +250,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 after + unsafety assignment. - 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; +}; >From a882d2fd7c64a636360a6d4c072572279ab0d6d6 Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Fri, 27 Oct 2023 13:42:13 +0800 Subject: [PATCH 2/2] refactor --- .../PreferMemberInitializerCheck.cpp | 91 +++++++++++-------- clang-tools-extra/docs/ReleaseNotes.rst | 4 +- 2 files changed, 56 insertions(+), 39 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp index ae91ae22612c40b..21cabb26060550e 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp @@ -61,38 +61,23 @@ AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) { return Node.getFieldIndex() >= Index; } -enum class AssignedLevel { None, Assigned, UnsafetyAssigned }; +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 -// 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 AssignedLevel isSafeAssignment(const FieldDecl *Field, const Expr *Init, - const CXXConstructorDecl *Context, - AssignedLevel HistoryLevel) { - if (HistoryLevel == AssignedLevel::UnsafetyAssigned) - return AssignedLevel::UnsafetyAssigned; - if (Field->getType()->isReferenceType() && - HistoryLevel == AssignedLevel::Assigned) - // assign to reference type twice cannot be simplified to once. - return AssignedLevel::UnsafetyAssigned; - - 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() - ? AssignedLevel::Assigned - : AssignedLevel::UnsafetyAssigned; +static bool canAdvanceAssignment(AssignedLevel Level) { + return Level == AssignedLevel::None || Level == AssignedLevel::Default; } static std::pair<const FieldDecl *, const Expr *> @@ -170,9 +155,46 @@ void PreferMemberInitializerCheck::check( std::map<const FieldDecl *, AssignedLevel> AssignedFields{}; + // 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. + auto UpdateAssignmentLevel = [Ctor, &AssignedFields](const FieldDecl *Field, + const Expr *Init) { + auto It = AssignedFields.find(Field); + if (It == AssignedFields.end()) + It = AssignedFields.insert_or_assign(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(Ctor))))); + const bool HasDependence = !match(expr(anyOf(MemberMatcher, DeclMatcher, + hasDescendant(MemberMatcher), + hasDescendant(DeclMatcher))), + *Init, Field->getASTContext()) + .empty(); + if (HasDependence) { + It->second = AssignedLevel::HasDependence; + return; + } + }; + for (const CXXCtorInitializer *Init : Ctor->inits()) if (FieldDecl *Field = Init->getMember()) - AssignedFields.insert({Field, AssignedLevel::Assigned}); + UpdateAssignmentLevel(Field, Init->getInit()); for (const Stmt *S : Body->body()) { if (S->getBeginLoc().isMacroID()) { @@ -198,13 +220,8 @@ void PreferMemberInitializerCheck::check( std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S, Ctor); if (!Field) continue; - const auto It = AssignedFields.find(Field); - AssignedLevel Level = AssignedLevel::None; - if (It != AssignedFields.end()) - Level = It->second; - Level = isSafeAssignment(Field, InitValue, Ctor, Level); - AssignedFields.insert_or_assign(Field, Level); - if (Level == AssignedLevel::UnsafetyAssigned) + UpdateAssignmentLevel(Field, InitValue); + if (!canAdvanceAssignment(AssignedFields[Field])) continue; const bool IsInDefaultMemberInitializer = IsUseDefaultMemberInitEnabled && getLangOpts().CPlusPlus11 && diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 5536f9e805d8f70..8214def7dc44a64 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -250,8 +250,8 @@ Changes in existing checks - Improved :doc:`cppcoreguidelines-prefer-member-initializer <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to - ignore delegate constructors and ignore re-assignment for reference or after - unsafety assignment. + 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits