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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:52
+
+  const std::vector<std::string> IngnoredValuesInput;
+  std::vector<int64_t> IngnoredValues;
----------------
`Ignored`. Please spellcheck throughout.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:33
+
+      return -3; // FILENOTFOUND
+   }
----------------
I notice you changed `-1` to `-3`. Is `return -1` not an example of the "no magic numbers" rule? If not, why not — and can you put something in the documentation about it?
At least add a unit test proving that `return -1;` is accepted without any diagnostic, if that's the intended behavior.


================
Comment at: test/clang-tidy/readability-magic-numbers.cpp:16
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 6 [readability-magic-numbers]
----------------
Please add test cases immediately following this one, for

    const int BadLocalConstInt = 6;
    constexpr int BadLocalConstexprInt = 6;
    static const int BadLocalStaticConstInt = 6;
    static constexpr int BadLocalStaticConstexprInt = 6;

(except of course changing "Bad" to "Good" in any cases where 6 is acceptable as an initializer).

Also

    std::vector<int> BadLocalVector(6);
    const std::vector<int> BadLocalConstVector(6);

etc. etc.


================
Comment at: test/clang-tidy/readability-magic-numbers.cpp:84
+void SolidFunction() {
+  const int GoodLocalIntContant = 43;
+
----------------
`Constant`. Please spellcheck throughout.


================
Comment at: test/clang-tidy/readability-magic-numbers.cpp:151
+
+const geometry::Rectangle<double> mandelbrotCanvas{geometry::Point<double>{-2.5, -1}, geometry::Dimension<double>{3.5, 2}};
----------------
Surely you should expect some diagnostics on this line!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114