[clang-tidy] Add a check for "magic numbers"
Needs ReviewPublic

Authored by 0x8000-0000 on Mon, Jul 9, 5:44 PM.

Details

Summary

Add a clang-tidy check for "magic numbers", integers and floating point values embedded in the code instead of using symbols or constants.

Bad example:

double circleArea = 3.1415926535 * radius * radius;
double totalCharge = 1.08 * itemPrice;

Good example:

double circleArea = M_PI * radius * radius;
double totalCharge = (1.0 + TAX_RATE) * itemPrice;

Diff Detail

There are a very large number of changes, so older changes are hidden. Show Older Changes
0x8000-0000 edited the summary of this revision. (Show Details)

Updated examples code and added several references

Eugene.Zelenko added inline comments.Wed, Jul 11, 4:08 PM
docs/ReleaseNotes.rst
174

Please make link short. See other links as examples.

docs/clang-tidy/checks/readability-magic-numbers.rst
30

This is not needed and also wrongly formatted.

47

This is not needed and also wrongly formatted.

Update documentation to clean up formatting

0x8000-0000 marked 3 inline comments as done.Wed, Jul 11, 4:41 PM

Thank you for your review @Eugene.Zelenko

Eugene.Zelenko added inline comments.Wed, Jul 11, 4:45 PM
docs/ReleaseNotes.rst
174

.html is not needed.

0x8000-0000 marked an inline comment as done.Wed, Jul 11, 5:06 PM

Remove extraneous .html. Sorry for not seeing it earlier.

Accept magic values arbitrarily deep in a constant initialization

@aaron.ballman , @JonasToth , @Eugene.Zelenko Is there anything missing from this patch? What do I need to do to get it merged? This is my first contribution to LLVM so I'm not quite sure. Thank you!

JonasToth added a comment.EditedTue, Jul 17, 7:27 AM

@aaron.ballman , @JonasToth , @Eugene.Zelenko Is there anything missing from this patch? What do I need to do to get it merged? This is my first contribution to LLVM so I'm not quite sure. Thank you!

Usually the review process takes a while ;) If you have currently time and are motivated you can have multiple patches in parallel and add aaron, hokein, alexfh as reviewers.

As long as there are outstanding comments, that review is not done and the reviewers consider it as not done.

For the general procedure:

  • propose the patch
  • Review happens, discuss everything and fix problems
  • review gets accepted with a LGTM (looks good to me) + a green symbol in the review (alexfh or aaron.ballman are usually the reviewers that accept. You should usually wait for their go)
  • patch gets commited. You most likely don't have the commit access yet. But if you plan to contribute more often you can request it.

For more information you can take a look at: https://llvm.org/docs/Contributing.html and https://llvm.org/docs/Phabricator.html and https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

clang-tidy/readability/MagicNumbersCheck.cpp
43

Use the type the function returns (what auto would deduce to).

clang-tidy/readability/MagicNumbersCheck.h
56

Maybe these vectors could be llvm::SmallVector that has a better performance.

docs/clang-tidy/checks/readability-magic-numbers.rst
12

There is an example in the cpp coreguidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es45-avoid-magic-constants-use-symbolic-constants

for (int m = 1; m <= 12; ++m)   // don't: magic constant 12
    cout << month[m] << '\n';

Maybe this could work?

Adding C++ Core Guidelines alias is definitely low-hanging fruit which could be implemented within this patch.

clang-tidy/readability/MagicNumbersCheck.cpp
15

Please remove empty line.

31

auto could be used here because it's range loop over container.

clang-tidy/readability/MagicNumbersCheck.h
15

Please remove empty line and also include <vector>.

test/clang-tidy/readability-magic-numbers.cpp
125

Please run Clang-format over test case.

0x8000-0000 marked 4 inline comments as done.Tue, Jul 17, 4:27 PM

@Eugene.Zelenko thank you for suggesting the alias - I didn't know it is that easy, but a grep on "alias" led me to the right place.

clang-tidy/readability/MagicNumbersCheck.h
56

The IgnoredValuesInput is parsed via utils::options::parseStringList which returns a std::vector<std::string>. I am using that instance directly - copying it into a llvm::SmallVector would be more expensive.

test/clang-tidy/readability-magic-numbers.cpp
125

I do run it constantly. In this case there is no change. I am using the clang-6.0 formatter though.

0x8000-0000 marked 13 inline comments as done.Tue, Jul 17, 4:28 PM
Eugene.Zelenko added inline comments.Tue, Jul 17, 4:37 PM
test/clang-tidy/readability-magic-numbers.cpp
125

It's weird, since closing curly bracket should be in same line as opening. Also Rectangle constructor is longer then 80 symbols.

Address several review comments. Create alias for cppcoreguidelines-avoid-magic-numbers .

0x8000-0000 marked an inline comment as done.Tue, Jul 17, 6:04 PM
0x8000-0000 added inline comments.
test/clang-tidy/readability-magic-numbers.cpp
125

If you download the code and run clang-format-6.0 do you get something different?

0x8000-0000 marked an inline comment as done.Tue, Jul 17, 6:09 PM

florin@helios:~/tools/llvm$ find . -name .clang-format | sort
./.clang-format
./projects/compiler-rt/lib/asan/.clang-format
./projects/compiler-rt/lib/dfsan/.clang-format
./projects/compiler-rt/lib/hwasan/.clang-format
./projects/compiler-rt/lib/interception/.clang-format
./projects/compiler-rt/lib/lsan/.clang-format
./projects/compiler-rt/lib/msan/.clang-format
./projects/compiler-rt/lib/safestack/.clang-format
./projects/compiler-rt/lib/sanitizer_common/.clang-format
./projects/compiler-rt/lib/tsan/.clang-format
./projects/libcxxabi/.clang-format
./projects/libcxx/.clang-format
./projects/libunwind/.clang-format
./projects/lld/.clang-format
./projects/openmp/runtime/.clang-format
./test/.clang-format
./tools/clang/.clang-format
./tools/clang/test/.clang-format
./tools/clang/tools/extra/test/.clang-format
florin@helios:~/tools/llvm$ cat ./tools/clang/tools/extra/test/.clang-format
BasedOnStyle: LLVM
ColumnLimit: 0

Doesn't seem that weird ;)

0x8000-0000 marked an inline comment as done.Tue, Jul 17, 6:10 PM
aaron.ballman added inline comments.Wed, Jul 18, 1:16 PM
clang-tidy/readability/MagicNumbersCheck.cpp
24

Why 2, 10, and 100?

31–33

This can be replaced with llvm::transform(IgnoredValuesInput, IgnoredValues.begin(), std::stoll);

34

Please use llvm::sort() instead to help find non-determinism bugs. (I'm a bit surprised we don't have a range-based sort though.)

38

IgnoredIntegerValues as the public facing option as well, unless you want to allow users to specify ignored floating-point values too (which could be useful as well).

42–43

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"))

55

Rather than indent everything for the typical case, I'd prefer this to be an early return.

73

This diagnostic text doesn't really tell the user what's wrong with their code or how to fix it. Also, this function is largely duplicated in checkFloatMatch(). I think they should be combined where possible and the diagnostic should read: '%0' is a magic number; consider replacing with a named constant or something along those lines.

119

Can use const auto * here as the type is spelled out in the initialization.

120–124

The if statements can be combined into a single statement and the braces can be elided.

125–127

Same here for auto and single statement.

136–141

return Node.get<SubstNonTypeTemplateParmExpr> != nullptr; However, that makes me wonder why this should be a function at all.

148

Elide braces

152–159

I think this could be replaced by a call to llvm::any_of()

165

No need for this local variable.

169–175

llvm::any_of()

184–187

No need for fancy C-style comments here; can just use //.

clang-tidy/readability/MagicNumbersCheck.h
16

This include can be removed.

54

I think this should be IgnoredIntegerValue given that it only is checked for integers. That should be reflected in the documentation as well.

clang-tidy/readability/ReadabilityTidyModule.cpp
69–70

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

docs/clang-tidy/checks/readability-magic-numbers.rst
2

This doesn't document the user-facing configuration options, but it should.

5

Underlining is too long.

7

literal -> literals

lebedev.ri added inline comments.
clang-tidy/readability/MagicNumbersCheck.cpp
24

These really should be a config option.

aaron.ballman added inline comments.Wed, Jul 18, 2:16 PM
clang-tidy/readability/MagicNumbersCheck.cpp
24

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.

0x8000-0000 added inline comments.Wed, Jul 18, 3:08 PM
clang-tidy/readability/MagicNumbersCheck.cpp
24

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.

Quuxplusone added inline comments.Wed, Jul 18, 4:46 PM
clang-tidy/readability/MagicNumbersCheck.h
53

Ignored. Please spellcheck throughout.

docs/clang-tidy/checks/readability-magic-numbers.rst
34

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.

test/clang-tidy/readability-magic-numbers.cpp
17

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.

85

Constant. Please spellcheck throughout.

152

Surely you should expect some diagnostics on this line!

0x8000-0000 marked 17 inline comments as done.Wed, Jul 18, 4:48 PM
0x8000-0000 added inline comments.
clang-tidy/readability/MagicNumbersCheck.cpp
31–33

/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?

42–43

addMatcher(anyOf(...), this) is ambiguous. Is there any benefit over the two statements?

clang-tidy/readability/ReadabilityTidyModule.cpp
69–70

I have it as an alias in my branch. I will check why it was not exported in the patch.

0x8000-0000 marked 8 inline comments as done.Wed, Jul 18, 5:06 PM
0x8000-0000 added inline comments.
docs/clang-tidy/checks/readability-magic-numbers.rst
34

-1 is parsed as "UnaryOperator" - "IntegerLiteral" 1. I think -1 should be acceptable by default.

test/clang-tidy/readability-magic-numbers.cpp
17

Again... all the "const .* (\d)+" patterns should be acceptable. We're initializing a constant. Would you prefer an explicit option?

152

I am using those values to initialize a constant, as described earlier.

We can disable this - but this will be a terribly noisy checker.

I think this requires a separate discussion - do we accept magic values only when they are used directly to initialize a constant of the same type? Or do we allow them generically when they are used to initialize any constant? Or do we only accept several layers of nesting, but not too much?

a) const int Foo = 42;
b) #define FROB 66
c) cont int Bar[] = { 43, 44 };
d) const geometry::Rectangle<double> mandelbrotCanvas{geometry::Point<double>{-2.5, -1}, geometry::Dimension<double>{3.5, 2}};

Which of these should produce a warning?

Significant refactoring to address review comments - mainly to reduce duplication and implement in functional style.

cppcoreguidelines-avoid-magic-numbers is documented as an alias to readability-magic-numbers check.

0x8000-0000 marked an inline comment as done.Wed, Jul 18, 7:18 PM
0x8000-0000 added inline comments.
test/clang-tidy/readability-magic-numbers.cpp
17

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.

aaron.ballman added inline comments.Thu, Jul 19, 5:13 AM
clang-tidy/readability/MagicNumbersCheck.cpp
24

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?

31–33

Yeah, I'd wrap in a lambda then.

42–43

Better memoization, which may not really be critical given how trivial the matchers are.

test/clang-tidy/readability-magic-numbers.cpp
17

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.

0x8000-0000 marked 3 inline comments as done.Thu, Jul 19, 3:00 PM
0x8000-0000 added inline comments.
test/clang-tidy/readability-magic-numbers.cpp
17

Ok - we have that already and there's nothing magic about std::vector or std::array.

Small refactoring and documentation update.

Revert built-in acceptable integers to -1, 0 and 1 and document them.

Add a (presently failing) test for not tripping up on LINE through several layers of macro expansion (as in GoogleTest library). This creates a lot of false positives in the unit tests and needs to be fixed.

0x8000-0000 updated this revision to Diff 156425.EditedThu, Jul 19, 9:20 PM

Filter out synthetic integers (such as _LINE_) from the report. All the tests are passing now.

0x8000-0000 added a comment.EditedThu, Jul 19, 10:33 PM
./tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py -clang-tidy-binary ../llvm.rel/bin/clang-tidy -checks="-*,readability-magic-numbers" -j 12 -p ../llvm.rel -j 12 -quiet > /tmp/llvm.magic
grep "warning:" /tmp/llvm.magic | cut -d: -f5 | cut -d" " -f2 | sort | uniq -c | sort -rn | head -40

With about 2000 files scanned (on the current llvm+clang+extra tree), these are the popularity rankings:

9559 2
5041 4
4359 8
2811 3
2555 16
1748 32
1213 64
1170 10
1155 128
 910 5
 853 6
 606 7
 537 256
 385 12
 382 15
 349 20
 322 1024
 288 255
 252 100
 238 9
 233 11
 181 40
 174 63
 170 24
 163 31
 161 512
 146 65535
 144 13
 136 14
 126 18
 120 1
 113 17
 103 1000
 100 4096
  92 60
  85 192
  84 30
  81 21
  76 25
  71 23

Total warning count is ~40K.

So it seems powers of 2 are popular, as are 10, 100, 1000.

2 occurs 25% of the time; 4+8+16 another 27%, while 32... 1024 occur 14%

Avoid parsing and reformatting the input literal - just print the original source code.