[clang-tools-extra] c16b3ec - Fix false positive in magic number checker.

[Aaron Ballman via cfe-commits]

Author: Florin Iucha
Date: 2019-12-24T10:03:00-05:00
New Revision: c16b3ec597d277b5a7397db308f8ec730f3330a3

URL: https://github.com/llvm/llvm-project/commit/c16b3ec597d277b5a7397db308f8ec730f3330a3
DIFF: https://github.com/llvm/llvm-project/commit/c16b3ec597d277b5a7397db308f8ec730f3330a3.diff

LOG: Fix false positive in magic number checker.

cppcoreguidelines-avoid-magic-numbers should not warn about enum class.
Fixes PR40640.

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

Modified: 
    clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
index 9114deb5c736..64806cee37ef 100644
--- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@ static bool isUsedToInitializeAConstant(const MatchFinder::MatchResult &Result,
     return AsDecl->isImplicit();
   }
 
-  if (Node.get<EnumConstantDecl>() != nullptr)
+  if (Node.get<EnumConstantDecl>())
     return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -125,8 +125,20 @@ bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
         if (isUsedToInitializeAConstant(Result, Parent))
           return true;
 
-        // Ignore this instance, because this match reports the location
-        // where the template is defined, not where it is instantiated.
+        // 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;
+                }))
+          return true;
+
+        // Ignore this instance, because this match reports the
+        // location where the template is defined, not where it
+        // is instantiated.
         if (Parent.get<SubstNonTypeTemplateParmExpr>())
           return true;
 

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a150cc6bf824..213b748f36f1 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@ Improvements to clang-tidy
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   <clang-tidy/checks/readability-make-member-function-const>` check.
 

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
new file mode 100644
index 000000000000..99d9be262a89
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t --
+// XFAIL: *
+
+int ProcessSomething(int input);
+
+int DoWork()
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+    return 0;
+
+   return 0;
+}
+
+

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
index 055ad76d09b5..06fdc6080c00 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@ class TwoIntContainer {
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
@@ -183,7 +183,7 @@ struct Rectangle {
 
 const geometry::Rectangle<double> mandelbrotCanvas{geometry::Point<double>{-2.5, -1}, geometry::Dimension<double>{3.5, 2}};
 
-// Simulate the macro magic in Google Test internal headers
+// Simulate the macro magic in Google Test internal headers.
 class AssertionHelper {
 public:
   AssertionHelper(const char *Message, int LineNumber) : Message(Message), LineNumber(LineNumber) {}
@@ -201,7 +201,7 @@ void FunctionWithCompilerDefinedSymbol(void) {
   ASSERTION_HELPER("here and now");
 }
 
-// prove that integer literals introduced by the compiler are accepted silently
+// Prove that integer literals introduced by the compiler are accepted silently.
 extern int ConsumeString(const char *Input);
 
 const char *SomeStrings[] = {"alpha", "beta", "gamma"};
@@ -215,3 +215,14 @@ int TestCheckerOverreach() {
 
   return Total;
 }
+
+// Prove that using enumerations values don't produce warnings.
+enum class Letter : unsigned {
+    A, B, C, D, E, F, G, H, I, J
+};
+
+template<Letter x> struct holder  { Letter letter = x;  };
+template<Letter x> struct wrapper { using h_type = holder<x>;  };
+
+template struct wrapper<Letter::A>;
+template struct wrapper<Letter::J>;