[clang-tidy] Remove enforcement of rule C.48 from cppcoreguidelines-prefer-member-init (#80330)

carlosgalvezp · Carlos Gálvez · web-flow · commit 6f32d6a4f362 · 2024-02-01T21:16:24.000+01:00
This functionality already exists in cppcoreguidelines-use-default-member-init. It was deprecated from this check in clang-tidy 17. This allows us to fully decouple this check from the corresponding modernize check, which has an unhealthy dependency. Fixes #62169 --------- Co-authored-by: Carlos Gálvez <carlos.galvez@zenseact.com>
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
@@ -34,27 +34,6 @@ static bool isNoReturnCallStatement(const Stmt *S) {
   return Func->isNoReturn();
 }
 
-static bool isLiteral(const Expr *E) {
-  return isa<StringLiteral, CharacterLiteral, IntegerLiteral, FloatingLiteral,
-             CXXBoolLiteralExpr, CXXNullPtrLiteralExpr>(E);
-}
-
-static bool isUnaryExprOfLiteral(const Expr *E) {
-  if (const auto *UnOp = dyn_cast<UnaryOperator>(E))
-    return isLiteral(UnOp->getSubExpr());
-  return false;
-}
-
-static bool shouldBeDefaultMemberInitializer(const Expr *Value) {
-  if (isLiteral(Value) || isUnaryExprOfLiteral(Value))
-    return true;
-
-  if (const auto *DRE = dyn_cast<DeclRefExpr>(Value))
-    return isa<EnumConstantDecl>(DRE->getDecl());
-
-  return false;
-}
-
 namespace {
 
 AST_MATCHER_P(FieldDecl, indexNotLessThan, unsigned, Index) {
@@ -166,19 +145,7 @@ isAssignmentToMemberOf(const CXXRecordDecl *Rec, const Stmt *S,
 
 PreferMemberInitializerCheck::PreferMemberInitializerCheck(
     StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      IsUseDefaultMemberInitEnabled(
-          Context->isCheckEnabled("modernize-use-default-member-init")),
-      UseAssignment(
-          Options.get("UseAssignment",
-                      OptionsView("modernize-use-default-member-init",
-                                  Context->getOptions().CheckOptions, Context)
-                          .get("UseAssignment", false))) {}
-
-void PreferMemberInitializerCheck::storeOptions(
-    ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "UseAssignment", UseAssignment);
-}
+    : ClangTidyCheck(Name, Context) {}
 
 void PreferMemberInitializerCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(cxxConstructorDecl(hasBody(compoundStmt()),
@@ -230,139 +197,99 @@ void PreferMemberInitializerCheck::check(
     updateAssignmentLevel(Field, InitValue, Ctor, AssignedFields);
     if (!canAdvanceAssignment(AssignedFields[Field]))
       continue;
-    const bool IsInDefaultMemberInitializer =
-        IsUseDefaultMemberInitEnabled && getLangOpts().CPlusPlus11 &&
-        Ctor->isDefaultConstructor() &&
-        (getLangOpts().CPlusPlus20 || !Field->isBitField()) &&
-        !Field->hasInClassInitializer() &&
-        (!isa<RecordDecl>(Class->getDeclContext()) ||
-         !cast<RecordDecl>(Class->getDeclContext())->isUnion()) &&
-        shouldBeDefaultMemberInitializer(InitValue);
-    if (IsInDefaultMemberInitializer) {
-      bool InvalidFix = false;
-      SourceLocation FieldEnd =
-          Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
-                                     *Result.SourceManager, getLangOpts());
-      InvalidFix |= FieldEnd.isInvalid() || FieldEnd.isMacroID();
-      SourceLocation SemiColonEnd;
-      if (auto NextToken = Lexer::findNextToken(
-              S->getEndLoc(), *Result.SourceManager, getLangOpts()))
-        SemiColonEnd = NextToken->getEndLoc();
-      else
-        InvalidFix = true;
-      auto Diag =
-          diag(S->getBeginLoc(), "%0 should be initialized in an in-class"
-                                 " default member initializer")
-          << Field;
-      if (InvalidFix)
-        continue;
-      CharSourceRange StmtRange =
-          CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
 
-      SmallString<128> Insertion(
-          {UseAssignment ? " = " : "{",
-           Lexer::getSourceText(Result.SourceManager->getExpansionRange(
-                                    InitValue->getSourceRange()),
-                                *Result.SourceManager, getLangOpts()),
-           UseAssignment ? "" : "}"});
-
-      Diag << FixItHint::CreateInsertion(FieldEnd, Insertion)
-           << FixItHint::CreateRemoval(StmtRange);
-
-    } else {
-      StringRef InsertPrefix = "";
-      bool HasInitAlready = false;
-      SourceLocation InsertPos;
-      SourceRange ReplaceRange;
-      bool AddComma = false;
-      bool InvalidFix = false;
-      unsigned Index = Field->getFieldIndex();
-      const CXXCtorInitializer *LastInListInit = nullptr;
-      for (const CXXCtorInitializer *Init : Ctor->inits()) {
-        if (!Init->isWritten() || Init->isInClassMemberInitializer())
-          continue;
-        if (Init->getMember() == Field) {
-          HasInitAlready = true;
-          if (isa<ImplicitValueInitExpr>(Init->getInit()))
-            InsertPos = Init->getRParenLoc();
-          else {
-            ReplaceRange = Init->getInit()->getSourceRange();
-          }
-          break;
-        }
-        if (Init->isMemberInitializer() &&
-            Index < Init->getMember()->getFieldIndex()) {
-          InsertPos = Init->getSourceLocation();
-          // There are initializers after the one we are inserting, so add a
-          // comma after this insertion in order to not break anything.
-          AddComma = true;
-          break;
+    StringRef InsertPrefix = "";
+    bool HasInitAlready = false;
+    SourceLocation InsertPos;
+    SourceRange ReplaceRange;
+    bool AddComma = false;
+    bool InvalidFix = false;
+    unsigned Index = Field->getFieldIndex();
+    const CXXCtorInitializer *LastInListInit = nullptr;
+    for (const CXXCtorInitializer *Init : Ctor->inits()) {
+      if (!Init->isWritten() || Init->isInClassMemberInitializer())
+        continue;
+      if (Init->getMember() == Field) {
+        HasInitAlready = true;
+        if (isa<ImplicitValueInitExpr>(Init->getInit()))
+          InsertPos = Init->getRParenLoc();
+        else {
+          ReplaceRange = Init->getInit()->getSourceRange();
         }
-        LastInListInit = Init;
+        break;
       }
-      if (HasInitAlready) {
-        if (InsertPos.isValid())
-          InvalidFix |= InsertPos.isMacroID();
-        else
-          InvalidFix |= ReplaceRange.getBegin().isMacroID() ||
-                        ReplaceRange.getEnd().isMacroID();
-      } else {
-        if (InsertPos.isInvalid()) {
-          if (LastInListInit) {
-            InsertPos = Lexer::getLocForEndOfToken(
-                LastInListInit->getRParenLoc(), 0, *Result.SourceManager,
-                getLangOpts());
-            // Inserting after the last constructor initializer, so we need a
-            // comma.
-            InsertPrefix = ", ";
-          } else {
-            InsertPos = Lexer::getLocForEndOfToken(
-                Ctor->getTypeSourceInfo()
-                    ->getTypeLoc()
-                    .getAs<clang::FunctionTypeLoc>()
-                    .getLocalRangeEnd(),
-                0, *Result.SourceManager, getLangOpts());
-
-            // If this is first time in the loop, there are no initializers so
-            // `:` declares member initialization list. If this is a
-            // subsequent pass then we have already inserted a `:` so continue
-            // with a comma.
-            InsertPrefix = FirstToCtorInits ? " : " : ", ";
-          }
-        }
+      if (Init->isMemberInitializer() &&
+          Index < Init->getMember()->getFieldIndex()) {
+        InsertPos = Init->getSourceLocation();
+        // There are initializers after the one we are inserting, so add a
+        // comma after this insertion in order to not break anything.
+        AddComma = true;
+        break;
+      }
+      LastInListInit = Init;
+    }
+    if (HasInitAlready) {
+      if (InsertPos.isValid())
         InvalidFix |= InsertPos.isMacroID();
+      else
+        InvalidFix |= ReplaceRange.getBegin().isMacroID() ||
+                      ReplaceRange.getEnd().isMacroID();
+    } else {
+      if (InsertPos.isInvalid()) {
+        if (LastInListInit) {
+          InsertPos =
+              Lexer::getLocForEndOfToken(LastInListInit->getRParenLoc(), 0,
+                                         *Result.SourceManager, getLangOpts());
+          // Inserting after the last constructor initializer, so we need a
+          // comma.
+          InsertPrefix = ", ";
+        } else {
+          InsertPos = Lexer::getLocForEndOfToken(
+              Ctor->getTypeSourceInfo()
+                  ->getTypeLoc()
+                  .getAs<clang::FunctionTypeLoc>()
+                  .getLocalRangeEnd(),
+              0, *Result.SourceManager, getLangOpts());
+
+          // If this is first time in the loop, there are no initializers so
+          // `:` declares member initialization list. If this is a
+          // subsequent pass then we have already inserted a `:` so continue
+          // with a comma.
+          InsertPrefix = FirstToCtorInits ? " : " : ", ";
+        }
       }
+      InvalidFix |= InsertPos.isMacroID();
+    }
 
-      SourceLocation SemiColonEnd;
-      if (auto NextToken = Lexer::findNextToken(
-              S->getEndLoc(), *Result.SourceManager, getLangOpts()))
-        SemiColonEnd = NextToken->getEndLoc();
+    SourceLocation SemiColonEnd;
+    if (auto NextToken = Lexer::findNextToken(
+            S->getEndLoc(), *Result.SourceManager, getLangOpts()))
+      SemiColonEnd = NextToken->getEndLoc();
+    else
+      InvalidFix = true;
+
+    auto Diag = diag(S->getBeginLoc(), "%0 should be initialized in a member"
+                                       " initializer of the constructor")
+                << Field;
+    if (InvalidFix)
+      continue;
+    StringRef NewInit = Lexer::getSourceText(
+        Result.SourceManager->getExpansionRange(InitValue->getSourceRange()),
+        *Result.SourceManager, getLangOpts());
+    if (HasInitAlready) {
+      if (InsertPos.isValid())
+        Diag << FixItHint::CreateInsertion(InsertPos, NewInit);
       else
-        InvalidFix = true;
-
-      auto Diag = diag(S->getBeginLoc(), "%0 should be initialized in a member"
-                                         " initializer of the constructor")
-                  << Field;
-      if (InvalidFix)
-        continue;
-      StringRef NewInit = Lexer::getSourceText(
-          Result.SourceManager->getExpansionRange(InitValue->getSourceRange()),
-          *Result.SourceManager, getLangOpts());
-      if (HasInitAlready) {
-        if (InsertPos.isValid())
-          Diag << FixItHint::CreateInsertion(InsertPos, NewInit);
-        else
-          Diag << FixItHint::CreateReplacement(ReplaceRange, NewInit);
-      } else {
-        SmallString<128> Insertion({InsertPrefix, Field->getName(), "(",
-                                    NewInit, AddComma ? "), " : ")"});
-        Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
-                                           FirstToCtorInits);
-        FirstToCtorInits = areDiagsSelfContained();
-      }
-      Diag << FixItHint::CreateRemoval(
-          CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
+        Diag << FixItHint::CreateReplacement(ReplaceRange, NewInit);
+    } else {
+      SmallString<128> Insertion({InsertPrefix, Field->getName(), "(", NewInit,
+                                  AddComma ? "), " : ")"});
+      Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
+                                         FirstToCtorInits);
+      FirstToCtorInits = areDiagsSelfContained();
     }
+    Diag << FixItHint::CreateRemoval(
+        CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
   }
 }
 
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.h
@@ -24,12 +24,8 @@ class PreferMemberInitializerCheck : public ClangTidyCheck {
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
-  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-
-  const bool IsUseDefaultMemberInitEnabled;
-  const bool UseAssignment;
 };
 
 } // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,14 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Cleaned up :doc:`cppcoreguidelines-prefer-member-initializer
+  <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`
+  by removing enforcement of rule `C.48
+  <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers>`_,
+  which was deprecated since :program:`clang-tidy` 17. This rule is now covered
+  by :doc:`cppcoreguidelines-use-default-member-init
+  <clang-tidy/checks/cppcoreguidelines/use-default-member-init>`.
+
 - Improved :doc:`modernize-avoid-c-arrays
   <clang-tidy/checks/modernize/avoid-c-arrays>` check by introducing the new
   `AllowStringArrays` option, enabling the exclusion of array types with deduced
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -13,27 +13,11 @@ This check implements `C.49
 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c49-prefer-initialization-to-assignment-in-constructors>`_
 from the C++ Core Guidelines.
 
-If the language version is `C++ 11` or above, the constructor is the default
-constructor of the class, the field is not a bitfield (only in case of earlier
-language version than `C++ 20`), furthermore the assigned value is a literal,
-negated literal or ``enum`` constant then the preferred place of the
-initialization is at the class member declaration.
-
-This latter rule is `C.48
+Please note, that this check does not enforce rule `C.48
 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers>`_
-from the C++ Core Guidelines.
-
-Please note, that this check does not enforce this latter rule for
-initializations already implemented as member initializers. For that purpose
+from the C++ Core Guidelines. For that purpose
 see check :doc:`modernize-use-default-member-init <../modernize/use-default-member-init>`.
 
-.. note::
-
-  Enforcement of rule C.48 in this check is deprecated, to be removed in
-  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
-  Please use :doc:`cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init>`
-  to enforce rule C.48.
-
 Example 1
 ---------
 
@@ -51,16 +35,16 @@ Example 1
     }
   };
 
-Here ``n`` can be initialized using a default member initializer, unlike
+Here ``n`` can be initialized in the constructor initializer list, unlike
 ``m``, as ``m``'s initialization follows a control statement (``if``):
 
 .. code-block:: c++
 
   class C {
-    int n{1};
+    int n;
     int m;
   public:
-    C() {
+    C(): n(1) {
       if (dice())
         return;
       m = 1;
@@ -84,7 +68,7 @@ Example 2
     }
   };
 
-Here ``n`` can be initialized in the constructor initialization list, unlike
+Here ``n`` can be initialized in the constructor initializer list, unlike
 ``m``, as ``m``'s initialization follows a control statement (``if``):
 
 .. code-block:: c++
@@ -94,29 +78,3 @@ Here ``n`` can be initialized in the constructor initialization list, unlike
       return;
     m = mm;
   }
-
-.. option:: UseAssignment
-
-   Note: this option is deprecated, to be removed in :program:`clang-tidy`
-   version 19. Please use the `UseAssignment` option from
-   :doc:`cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init>`
-   instead.
-
-   If this option is set to `true` (by default `UseAssignment` from
-   :doc:`modernize-use-default-member-init
-   <../modernize/use-default-member-init>` will be used),
-   the check will initialize members with an assignment.
-   In this case the fix of the first example looks like this:
-
-.. code-block:: c++
-
-  class C {
-    int n = 1;
-    int m;
-  public:
-    C() {
-      if (dice())
-        return;
-      m = 1;
-    }
-  };
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init-assignment.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init-assignment.cpp
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init.cpp
