- User Since
- Jul 9 2018, 5:24 PM (80 w, 1 d)
Sun, Jan 12
As an aside, once this is merged in, I dream of a "fix-it" for old style C code:
Sat, Jan 11
One more mis-constification that I can't explain, nor reduce to a small test case (when I extract the code, the check behaves as expected / desired)
Summary of "misses"
I have built diff14 and tried to apply it on an internal code base with ~7500 translation units.
Fri, Jan 10
Here is a minimal example that shows the problem:
Applied the fix-it on one of our smaller (but C++ code bases) and it builds and passes the tests. Comments:
Mon, Jan 6
Sun, Jan 5
I can't build this on top of current llvm-project tree (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7):
Sat, Jan 4
This patch does not build on top of current tree:
Fri, Jan 3
Indicated where the new test code should go.
Tue, Dec 31
Mon, Dec 30
Dec 22 2019
@aaron.ballman updated as suggested; please commit/integrate when you have a moment. Thank you!
Minor comment fixes; capitalization, full stop.
Dec 21 2019
My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.
Dec 20 2019
Add a test file for expected failures
Dec 19 2019
Update release notes.
Dec 18 2019
Dec 8 2019
@aaron.ballman - can you please review this patch and if you find it acceptable please integrate it?
Dec 7 2019
Small documentation fixes.
Dec 6 2019
Flipped the default to ignore bit fields. Moved static functions out of the anonymous namespace into the clang namespace.
Dec 5 2019
Updated release notes. Removed empty lines.
Document the new 'IgnoreBitFieldsWidths' option.
Dec 4 2019
Simplified the test cases by removing extraneous code.
Dec 3 2019
Dec 2 2019
Thank you for rebasing on current master.
Sep 6 2019
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.