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

[Florin Iucha via Phabricator via cfe-commits]

0x8000-0000 marked 17 inline comments as done.
0x8000-0000 added inline comments.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:30-32
+  for (const std::string &IgnoredValue : IngnoredValuesInput) {
+    IngnoredValues.push_back(std::stoll(IgnoredValue));
+  }
----------------
aaron.ballman wrote:
> This can be replaced with `llvm::transform(IgnoredValuesInput, IgnoredValues.begin(), std::stoll);`
/home/florin/tools/llvm/tools/clang/tools/extra/clang-tidy/readability/MagicNumbersCheck.cpp:30:3: error: no matching function for call to 'transform'                                        
  llvm::transform(IgnoredValuesInput, IgnoredIntegerValues.begin(), std::stoll);
  ^~~~~~~~~~~~~~~
/home/florin/tools/llvm/include/llvm/ADT/STLExtras.h:990:10: note: candidate template ignored: couldn't infer template argument 'UnaryPredicate'                                              
OutputIt transform(R &&Range, OutputIt d_first, UnaryPredicate P) {
         ^
1 error generated.

Shall I wrap it in a lambda?


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:41-42
+void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("ii"), this);
+  Finder->addMatcher(floatLiteral().bind("ff"), this);
+}
----------------
aaron.ballman wrote:
> The names `ii` and `ff` could be a bit more user-friendly. Also, this can be written using a single matcher, I think.
> `anyOf(integerLiteral().bind("integer"), floatLiteral().bind("float"))`
addMatcher(anyOf(...), this) is ambiguous. Is there any benefit over the two statements?


================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:69-70
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<MagicNumbersCheck>(
+        "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
----------------
aaron.ballman wrote:
> I think this should also be registered as a C++ Core Guideline checker, as I believe it covers at least the integer and floating-point parts of https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-magic
I have it as an alias in my branch.  I will check why it was not exported in the patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114