[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/readability/MagicNumbersCheck.cpp:23
+
+const char DefaultIgnoredValues[] = "0;1;2;10;100;";
+
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > Why 2, 10, and 100?
> > > These really should be a config option.
> > These are the default values for the config option. I think 0 and 1 make a lot of sense to have in the default list given how prevalent they are in real world code. But the other three seem to be used far less often.
> > 
> > I think if we wanted to have any values other than 0 and 1 (and perhaps -1) as defaults, I'd want to see some data on large amounts of code to justify anything else.
> No need for -1, as it is covered by 1, plus unary operator.
> 
> I know 100 is used often for converting percentages.
> 
> 10 is used for number parsing (yes, many people still do that by hand instead of libraries).
> 
> But this is the configuration - and I'm ok with reverting to 0 and 1 as I had originally.
Good point on -1. I'd say let's revert until we have statistics to justify any other default values. That said, I'd still be curious to know how chatty this is over large code bases. How many times does this trigger in LLVM, for instance?


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:30-32
+  for (const std::string &IgnoredValue : IngnoredValuesInput) {
+    IngnoredValues.push_back(std::stoll(IgnoredValue));
+  }
----------------
0x8000-0000 wrote:
> 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?
Yeah, I'd wrap in a lambda then.


================
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);
+}
----------------
0x8000-0000 wrote:
> 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?
Better memoization, which may not really be critical given how trivial the matchers are.


================
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]
----------------
0x8000-0000 wrote:
> 0x8000-0000 wrote:
> > Quuxplusone wrote:
> > > 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.
> > Again... all the "const .* (\d)+" patterns should be acceptable. We're initializing a constant. Would you prefer an explicit option?
> I have  template and constructor arguments already in the test. I have tried including <vector> but somehow it is not found and the std::vector is reported as an error in itself.
Tests need to be hermetic and cannot rely on STL headers or other system headers. Basically, you have to replicate the contents of <vector> (or whatever) within the test file for whatever you're trying to test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114