[clang-tidy] Change readability-magic-numbers to allow 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]

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

41

This should be configurable, someone else may don't want magic numbers in template arguments.
So we shouldn't change default behaviour.

clang-tools-extra/docs/ReleaseNotes.rst

196

consider: "check, now allows..."

[PiotrZSL]

This is for issue: https://github.com/llvm/llvm-project/issues/61259
Reference it in commit message.

Add config: "flag should be added i.e. IgnoreTypeAliases which will default to false."
Except that looks fine.

[florianhumblot]

Made it so that the proposed change doesn't change the default behavior of the check.
Introduce an option to allow enabling the exclusion.

[PiotrZSL]

"Context not available." - use arcanist (just merge all local commits into one), or generate diff with full context (diff -U 9999999).

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

58

example put under "Example with magic values refactored:" should be by default as (+-):

using containerType = CustomType<int, NUMBER_OF_ELEMENTS>;

as this is required by default.

[florianhumblot]

Generate diff with full context and updated the example to use the default settings of the readabiliy-magic-numbers checker

[PiotrZSL]

LGTM

[florianhumblot]

In D145778#4184530, @PiotrZSL wrote:

LGTM

Thanks for the reviews @PiotrZSL !
Is one review enough or does the second reviewer also need to approve these changes?

If this one is enough, could I ask you (or someone else) to commit the change since I don't have commit access? :)

[PiotrZSL]

What should I put as commit author ?
I'm asking because this is diff, and commit author looks to be attached to it.

[florianhumblot]

In D145778#4184611, @PiotrZSL wrote:

What should I put as commit author ?
I'm asking because this is diff, and commit author looks to be attached to it.

"Florian Humblot <[e-mail]>" should be good :)

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 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;
 
   return llvm::any_of(Result.Context->getParents(Node),
                       [&Result](const DynTypedNode &Parent) {
                         return isUsedToDefineABitField(Result, Parent);
                       });
 }
 
 namespace tidy::readability {
 
 const char DefaultIgnoredIntegerValues[] = "1;2;3;4;";
 const char DefaultIgnoredFloatingPointValues[] = "1.0;100.0;";
 
 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
   <clang-tidy/checks/readability/container-size-empty>` check when comparing
   ``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, now allows for
+  magic numbers in type aliases such as ``using`` and ``typedef`` declarations if
+  the new ``IgnoreTypeAliases`` option is set to true.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

* Rule 20701 in "TRAIN REAL TIME DATA PROTOCOL Coding Rules" by Armin-Hagen
      Weiss, Bombardier
    * http://wiki.c2.com/?MagicNumber
 
 
 Examples of magic values:
 
 .. code-block:: c++
 
+   template<typename T, size_t N>
+   struct CustomType {
+      T arr[N];
+   };
+
+   struct OtherType {
+      CustomType<int, 30> container;
+   }
+   CustomType<int, 30> values;
+
    double circleArea = 3.1415926535 * radius * radius;
 
    double totalCharge = 1.08 * itemPrice;
 
    int getAnswer() {
       return -3; // FILENOTFOUND
    }
 
    for (int mm = 1; mm <= 12; ++mm) {
       std::cout << month[mm] << '\n';
    }
 
 Example with magic values refactored:
 
 .. code-block:: c++
 
+   template<typename T, size_t N>
+   struct CustomType {
+      T arr[N];
+   };
+
+   const size_t NUMBER_OF_ELEMENTS = 30;
+   using containerType = CustomType<int, NUMBER_OF_ELEMENTS>;
+   
+   struct OtherType {
+      containerType container;
+   }
+   containerType values;
+
    double circleArea = M_PI * radius * radius;
 
    const double TAX_RATE = 0.08;  // or make it variable and read from a file
 
    double totalCharge = (1.0 + TAX_RATE) * itemPrice;
 
    int getAnswer() {
       return E_FILE_NOT_FOUND;
.. 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.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
  */
 
 #define INT_MACRO 5
 
 const int GoodGlobalIntConstant = 42;