[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:45-46
         "cppcoreguidelines-interfaces-global-init");
+    CheckFactories.registerCheck<readability::MagicNumbersCheck>(
+        "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
----------------
Please keep this alphabetized in the list.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:53
+
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+
----------------
Is the trailing semicolon after the `1` necessary?


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:88
+               // where the template is defined, not where it is instantiated.
+               (Parent.get<SubstNonTypeTemplateParmExpr>() != nullptr);
+      });
----------------
No need to use `!= nullptr`, or having the parens for that expression.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:109
+      SourceManager->getDecomposedLoc(Literal->getLocation());
+  if (FileOffset.first.isInvalid()) {
+    return false;
----------------
Can elide the braces.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:49
+  void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result,
+                       const char *boundName) {
+    const L *MatchedLiteral = Result.Nodes.getNodeAs<L>(boundName);
----------------
boundName -> BoundName per naming conventions.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:51
+    const L *MatchedLiteral = Result.Nodes.getNodeAs<L>(boundName);
+    if (MatchedLiteral == nullptr)
+      return;
----------------
`!MatchedLiteral`


================
Comment at: docs/clang-tidy/checks/list.rst:82
    cppcoreguidelines-avoid-goto
+   cppcoreguidelines-avoid-magic-numbers
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
----------------
This should have a `(redirects to blah)` blurb after it.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:12
+
+   * Item 17 in "C++ Coding Standards: 101 Rules, Guidelines and Best
+     Practices" by Herb Sutter and Andrei Alexandrescu
----------------
Given that the C++ Core Guideline check redirects here, it would be good to link to the correct place in those guidelines from here.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.
----------------
I am curious to know how true this is. You got some data for integer values and reported it, but I'm wondering if you've tried the same experiment with floating-point numbers?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114