[clang-tools-extra] [clang-tidy]Fix PreferMemberInitializer false positive for reassignment (PR #70316)
Congcong Cai via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 26 22:42:27 PDT 2023
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 <congcongcai0907 at 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 <congcongcai0907 at 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
More information about the cfe-commits
mailing list