[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

40

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

   const bool IgnoreAllFloatingPointValues;
   const bool IgnoreBitFieldsWidths;
   const bool IgnorePowersOf2IntegerValues;
+  const bool IgnoreTypeAliases;
   const StringRef RawIgnoredIntegerValues;
   const StringRef RawIgnoredFloatingPointValues;
 

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

 #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>

                       });
 }
 
+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>();

       IgnoreBitFieldsWidths(Options.get("IgnoreBitFieldsWidths", true)),
       IgnorePowersOf2IntegerValues(
           Options.get("IgnorePowersOf2IntegerValues", false)),
+      IgnoreTypeAliases(Options.get("IgnoreTypeAliases", false)),
       RawIgnoredIntegerValues(
           Options.get("IgnoredIntegerValues", DefaultIgnoredIntegerValues)),
       RawIgnoredFloatingPointValues(Options.get(

   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);

                                    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>() &&

clang-tools-extra/docs/ReleaseNotes.rst

   <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
 ^^^^^^^^^^^^^^
 

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

 
 .. 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;

 
 .. code-block:: c++
 
+   template<typename T, size_t N>
+   struct CustomType {
+      T arr[N];
+   };
+
+   using containerType = CustomType<int, 30>;
+   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

    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:  [{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>

    // 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