[clang-tidy] Introduce option to allow magic numbers in type aliases

[florianhumblot]

Summary

This commit introduces an option to the readability-magic-values checker.
The option defaults to false so that the behavior of the checker doesn't change unless specifically enabled.
These commits are supposed to fix https://github.com/llvm/llvm-project/issues/61259

[PiotrZSL]

Merge those commits under D145778, no need for separate review. (git rebase -i HEAD~3)

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

198

is set to true, not false.

[florianhumblot]

Accidentally opened this one with arcanist. The changes have been moved to https://reviews.llvm.org/D145778

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h

void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result,
     diag(MatchedLiteral->getLocation(),
          "%0 is a magic number; consider replacing it with a named constant")
         << LiteralSourceText;
   }
 
   const bool IgnoreAllFloatingPointValues;
   const bool IgnoreBitFieldsWidths;
   const bool IgnorePowersOf2IntegerValues;
+  const bool IgnoreTypeAliases;
   const StringRef RawIgnoredIntegerValues;
   const StringRef RawIgnoredFloatingPointValues;
 
   constexpr static unsigned SensibleNumberOfMagicValueExceptions = 16;
 
   constexpr static llvm::APFloat::roundingMode DefaultRoundingMode =
       llvm::APFloat::rmNearestTiesToEven;
 

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

 //===--- MagicNumbersCheck.cpp - clang-tidy-------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // A checker for magic numbers: integer or floating point literals embedded
 // in the code, outside the definition of a constant or an enumeration.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MagicNumbersCheck.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/STLExtras.h"
 #include <algorithm>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 
 static bool isUsedToInitializeAConstant(const MatchFinder::MatchResult &Result,
                                         const DynTypedNode &Node) {
 
   const auto *AsDecl = Node.get<DeclaratorDecl>();
   if (AsDecl) {
     if (AsDecl->getType().isConstQualified())
       return true;
 
     return AsDecl->isImplicit();
   }
 
   if (Node.get<EnumConstantDecl>())
     return true;
 
+  return llvm::any_of(Result.Context->getParents(Node),
+                      [&Result](const DynTypedNode &Parent) {
+                        return isUsedToInitializeAConstant(Result, Parent);
+                      });
+}
+
+static bool isUsedToDefineATypeAlias(const MatchFinder::MatchResult &Result,
+                                     const DynTypedNode &Node) {
+
   if (Node.get<TypeAliasDecl>() || Node.get<TypedefNameDecl>())
     return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
                       [&Result](const DynTypedNode &Parent) {
-                        return isUsedToInitializeAConstant(Result, Parent);
+                        return isUsedToDefineATypeAlias(Result, Parent);
                       });
 }
 
 static bool isUsedToDefineABitField(const MatchFinder::MatchResult &Result,
                                     const DynTypedNode &Node) {
   const auto *AsFieldDecl = Node.get<FieldDecl>();
   if (AsFieldDecl && AsFieldDecl->isBitField())
     return true;

 
 MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreAllFloatingPointValues(
           Options.get("IgnoreAllFloatingPointValues", false)),
       IgnoreBitFieldsWidths(Options.get("IgnoreBitFieldsWidths", true)),
       IgnorePowersOf2IntegerValues(
           Options.get("IgnorePowersOf2IntegerValues", false)),
+      IgnoreTypeAliases(Options.get("IgnoreTypeAliases", false)),
       RawIgnoredIntegerValues(
           Options.get("IgnoredIntegerValues", DefaultIgnoredIntegerValues)),
       RawIgnoredFloatingPointValues(Options.get(
           "IgnoredFloatingPointValues", DefaultIgnoredFloatingPointValues)) {
   // Process the set of ignored integer values.
   const std::vector<StringRef> IgnoredIntegerValuesInput =
       utils::options::parseStringList(RawIgnoredIntegerValues);
   IgnoredIntegerValues.resize(IgnoredIntegerValuesInput.size());

 }
 
 void MagicNumbersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreAllFloatingPointValues",
                 IgnoreAllFloatingPointValues);
   Options.store(Opts, "IgnoreBitFieldsWidths", IgnoreBitFieldsWidths);
   Options.store(Opts, "IgnorePowersOf2IntegerValues",
                 IgnorePowersOf2IntegerValues);
+  Options.store(Opts, "IgnoreTypeAliases", IgnoreTypeAliases);
   Options.store(Opts, "IgnoredIntegerValues", RawIgnoredIntegerValues);
   Options.store(Opts, "IgnoredFloatingPointValues",
                 RawIgnoredFloatingPointValues);
 }
 
 void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(integerLiteral().bind("integer"), this);
   if (!IgnoreAllFloatingPointValues)
     Finder->addMatcher(floatLiteral().bind("float"), this);
 }
 
 void MagicNumbersCheck::check(const MatchFinder::MatchResult &Result) {
 
   TraversalKindScope RAII(*Result.Context, TK_AsIs);
 
   checkBoundMatch<IntegerLiteral>(Result, "integer");
   checkBoundMatch<FloatingLiteral>(Result, "float");
 }
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
                                    const Expr &ExprResult) const {
   return llvm::any_of(
       Result.Context->getParents(ExprResult),
-      [&Result](const DynTypedNode &Parent) {
+      [this, &Result](const DynTypedNode &Parent) {
         if (isUsedToInitializeAConstant(Result, Parent))
           return true;
 
+        if (IgnoreTypeAliases && isUsedToDefineATypeAlias(Result, Parent))
+          return true;
+
         // Ignore this instance, because this matches an
         // expanded class enumeration value.
         if (Parent.get<CStyleCastExpr>() &&
             llvm::any_of(
                 Result.Context->getParents(Parent),
                 [](const DynTypedNode &GrandParent) {
                   return GrandParent.get<SubstNonTypeTemplateParmExpr>() !=
                          nullptr;

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed a false positive in :doc:`readability-container-size-empty
   ``std::array`` objects to default constructed ones. The behavior for this and
   other relevant classes can now be configured with a new option.
 
 - Improved :doc:`bugprone-too-small-loop-variable
   <clang-tidy/checks/bugprone/too-small-loop-variable>` check. Basic support
   for bit-field and integer members as a loop variable or upper limit were added.
 
 - Improved :doc:`readability-magic-numbers 
-  <clang-tidy/checks/readability/magic-numbers>` check. The check now allows for
-  magic numbers in type aliases such as ``using`` and ``typedef`` declarations.
+  <clang-tidy/checks/readability/magic-numbers>` check, now allows for
+  magic numbers in type aliases such as ``using`` and ``typedef`` declarations if
+  the new ``IgnoreTypeAliases`` option is set to false.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...

clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst

.. option:: IgnoreAllFloatingPointValues
    Boolean value indicating whether to accept all floating point values without
    warning. Default value is `false`.
 
 .. option:: IgnoreBitFieldsWidths
 
    Boolean value indicating whether to accept magic numbers as bit field widths
    without warning. This is useful for example for register definitions which
    are generated from hardware specifications. Default value is `true`.
+
+.. option:: IgnoreTypeAliases
+
+   Boolean value indicating whether to accept magic numbers in ``typedef`` or
+   ``using`` declarations. Default value is `false`

clang-tools-extra/test/clang-tidy/checkers/readability/magic-numbers-type-aliases.cpp

+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoreTypeAliases, value: true}]}' \
+// RUN: --
+
+template <typename T, int V>
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+/**
+ * With the flag enabled these should produce no warning
+ */
+using NumberInTypeAlias = ValueBucket<int, 25>;
+typedef ValueBucket<char, 243> NumberInTypedef;

clang-tools-extra/test/clang-tidy/checkers/readability/magic-numbers.cpp

 // RUN: %check_clang_tidy %s readability-magic-numbers %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
 // RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;10000.0;101.0;0x1.2p3"}, \
 // RUN:   {key: readability-magic-numbers.IgnoreBitFieldsWidths, value: false}, \
-// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: true}]}' \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: true}, \
+// RUN:   {key: readability-magic-numbers.IgnoreTypeAliases, value: false}]}' \
 // RUN: --
 
 template <typename T, int V>
 struct ValueBucket {
   T value[V];
 };
 
 int BadGlobalInt = 5;
struct HardwareGateway {
    unsigned int: 0;      // no warning since this is suppressed by IgnoredIntegerValues rule
    unsigned int Rest: 13;
    // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 13 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
    //
    unsigned int Another[3];
    // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 };
 
+using NumberInTypeAlias = ValueBucket<int, 25>;
+// CHECK-MESSAGES: :[[@LINE-1]]:44: warning: 25 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+typedef ValueBucket<char, 243> NumberInTypedef;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 243 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
 /*
  * Clean code
  */
 
-/*
- * No warning for type aliases
- */
-using NumberInTypeAlias = ValueBucket<int, 25>;
-typedef ValueBucket<char, 243> NumberInTypedef;
 
 #define INT_MACRO 5
 
 const int GoodGlobalIntConstant = 42;
 
 constexpr int AlsoGoodGlobalIntConstant = 42;
 
 int InitializedByMacro = INT_MACRO;