- User Since
- Jul 9 2018, 5:24 PM (61 w, 6 d)
Fri, Sep 6
Looks good to me. Thank you for the fix.
Aug 12 2018
Aug 11 2018
Rebased on curent master.
Aug 5 2018
Address several style comments. Rebase to current trunk (8.0).
Jul 31 2018
Add reference to 5.1.1 Use symbolic names instead of literal values in code in the documentation.
Add tests to show that we can detect, report and ignore floating point numbers represented using hexadecimal notation
The state of this patch:
- user interface is as agreed-upon, giving end-users the capability to filter out floats and ints as it makes sense for their code base
- code is clean
- documentation is up to date
- default ignore lists are sensible
Jul 29 2018
Update the list of magic values ignored by default.
Based on this, I think the integer list should also include 2, 3, and 4 as defaults -- those show up a lot more than I'd have expected. As for floating-point values, 1.0 certainly jumps out at me, but none of the rest seem particularly common. What do you think?
Top 40 magic numbers in https://github.com/qt/qtbase
4859 2 2901 3 1855 4 985 5 968 8 605 6 600 7 439 16 432 10 363 356 32 241 1.0f 217 12 209 255 207 100 205 9 205 20 204 50 177 0.5 174 15 162 0x2 144 24 140 0x80 135 11 127 256 113 14 110 0xff 101 1.0 99 64 99 200 96 13 86 30 84 1000 68 18 66 150 62 127 62 0xFF 58 19 58 0.05f 57 128
Add a flag to ignore all powers of two integral values.
See inline comments. Basically we need two arrays because APFloats of different semantics don't compare well, and even if we coerce them, they sometimes are not equal.
Indicate that 0 and 0.0 are accepted unconditionally (because it makes sense in the source code, and speeds-up many checks as 0s are very common and we don't want to spend log2(n) to find them at the beginning of the vector).
Jul 28 2018
Add support for ignoring specific floating point numbers.
Not trying to be difficult here - I have attempted to implement the straight-forward check.
Add option to ignore all floating point values.
I will add one more option: IgnoreFloatingPointValues, to ignore all floats and doubles, because the FloatingLiteral does not distinguish between them (as implementation detail), and I don't see a good reason to be strict about doubles and lenient about floats, or viceversa. The default value for this option is false.
Address review comments to improve documentation and readability
Jul 26 2018
Remove extra verbose namespaces and update documentation to indicate why -1 is accepted even when not explicitly called out.
Jul 24 2018
Update links in documentation
Fix a few typos
@aaron.ballman , @JonasToth , @Eugene.Zelenko - would you be so kind to do one more review pass and let me know if there are any blockers or if this is ready to merge? I do not have commit privileges, so if it is alright, feel free to merge it.
Bail out early if a [grand-]parent node is a declaration, but not a constant declaration.
Search for enumerations and declarations in the same tree traversal.
Jul 23 2018
Ignore literals implicitly added by the compiler, such as when using a range-for loop over a constant array.
Jul 19 2018
Avoid parsing and reformatting the input literal - just print the original source code.
./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
Filter out synthetic integers (such as _LINE_) from the report. All the tests are passing now.
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.
Small refactoring and documentation update.
Jul 18 2018
Any plans for fix-its that will add the suggested 'const' qualifiers?
Significant refactoring to address review comments - mainly to reduce duplication and implement in functional style.
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?
Jul 17 2018
florin@helios:~/tools/llvm$ find . -name .clang-format | sort
florin@helios:~/tools/llvm$ cat ./tools/clang/tools/extra/test/.clang-format
Address several review comments. Create alias for cppcoreguidelines-avoid-magic-numbers .
@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.
Jul 14 2018
Jul 13 2018
Accept magic values arbitrarily deep in a constant initialization
Jul 11 2018
Remove extraneous .html. Sorry for not seeing it earlier.
Thank you for your review @Eugene.Zelenko
Update documentation to clean up formatting
Updated examples code and added several references
Jul 10 2018
Incorporate review comments. Add support for floating point detection.
Jul 9 2018
Perhaps M_PI wasn't the best example, as its value won't change soon, but other numbers should be defined in relation to constants.