[libc-commits] [libc] [clang-tidy]Fix PreferMemberInitializer false positive for reassignment (PR #70316)

Congcong Cai via libc-commits libc-commits at lists.llvm.org
Fri Oct 27 08:45:34 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/3] [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/3] 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

>From c87e0f80d838fe9f5d1fa12db85ac373d4bbe853 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Fri, 27 Oct 2023 23:45:12 +0800
Subject: [PATCH 3/3] refactor

---
 .../PreferMemberInitializerCheck.cpp          | 83 ++++++++++---------
 1 file changed, 42 insertions(+), 41 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
index 21cabb26060550e..b6daf8b936bde0f 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
@@ -11,7 +11,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
-#include <map>
+#include "llvm/ADT/DenseMap.h"
 
 using namespace clang::ast_matchers;
 
@@ -80,6 +80,44 @@ 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 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(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 *>
 isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S,
                        const CXXConstructorDecl *Ctor) {
@@ -153,48 +191,11 @@ void PreferMemberInitializerCheck::check(
   const CXXRecordDecl *Class = Ctor->getParent();
   bool FirstToCtorInits = true;
 
-  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;
-    }
-  };
+  llvm::DenseMap<const FieldDecl *, AssignedLevel> AssignedFields{};
 
   for (const CXXCtorInitializer *Init : Ctor->inits())
     if (FieldDecl *Field = Init->getMember())
-      UpdateAssignmentLevel(Field, Init->getInit());
+      updateAssignmentLevel(Field, Init->getInit(), Ctor, AssignedFields);
 
   for (const Stmt *S : Body->body()) {
     if (S->getBeginLoc().isMacroID()) {
@@ -220,7 +221,7 @@ void PreferMemberInitializerCheck::check(
     std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S, Ctor);
     if (!Field)
       continue;
-    UpdateAssignmentLevel(Field, InitValue);
+    updateAssignmentLevel(Field, InitValue, Ctor, AssignedFields);
     if (!canAdvanceAssignment(AssignedFields[Field]))
       continue;
     const bool IsInDefaultMemberInitializer =



More information about the libc-commits mailing list