llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) <details> <summary>Changes</summary> - assignment twice cannot be simplified to once when assigning to reference type - safe assignment cannot be advanced before unsafe assignment --- Full diff: https://github.com/llvm/llvm-project/pull/70316.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp (+36-10) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1) - (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer.cpp (+16) ``````````diff 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; +}; `````````` </details> https://github.com/llvm/llvm-project/pull/70316 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
