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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
