Page MenuHomePhabricator

0x8000-0000 (Florin Iucha)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 9 2018, 5:24 PM (22 w, 4 d)

Recent Activity

Aug 12 2018

0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

Alex has had plenty of time to respond, so I'm fine handling any concerns he has post-commit. Do you need me to commit this on your behalf?

Aug 12 2018, 7:05 AM

Aug 11 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Rebased on curent master.

Aug 11 2018, 12:27 PM

Aug 5 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Address several style comments. Rebase to current trunk (8.0).

Aug 5 2018, 9:07 AM

Jul 31 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Add reference to 5.1.1 Use symbolic names instead of literal values in code in the documentation.

Jul 31 2018, 7:05 PM
0x8000-0000 updated the summary of D49114: [clang-tidy] Add a check for "magic numbers".
Jul 31 2018, 7:02 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Add tests to show that we can detect, report and ignore floating point numbers represented using hexadecimal notation

Jul 31 2018, 5:15 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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 31 2018, 4:45 AM

Jul 29 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Update the list of magic values ignored by default.

Jul 29 2018, 1:15 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 12:58 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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?

Jul 29 2018, 11:58 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 11:46 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 11:29 AM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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
Jul 29 2018, 11:26 AM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Add a flag to ignore all powers of two integral values.

Jul 29 2018, 10:27 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 9:43 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 9:31 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 9:14 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 8:56 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 8:27 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 29 2018, 7:22 AM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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.

Jul 29 2018, 6:51 AM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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 29 2018, 6:43 AM

Jul 28 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Add support for ignoring specific floating point numbers.

Jul 28 2018, 7:00 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

Not trying to be difficult here - I have attempted to implement the straight-forward check.

Jul 28 2018, 6:27 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 28 2018, 1:31 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 28 2018, 11:23 AM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Add option to ignore all floating point values.

Jul 28 2018, 11:17 AM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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.

Jul 28 2018, 10:53 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 28 2018, 10:47 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 28 2018, 10:46 AM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 28 2018, 10:13 AM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Address review comments to improve documentation and readability

Jul 28 2018, 10:07 AM

Jul 26 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Remove extra verbose namespaces and update documentation to indicate why -1 is accepted even when not explicitly called out.

Jul 26 2018, 5:23 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 26 2018, 5:14 PM

Jul 24 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Update links in documentation

Jul 24 2018, 7:17 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Fix a few typos

Jul 24 2018, 7:06 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

@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.

Jul 24 2018, 6:56 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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 24 2018, 4:18 AM

Jul 23 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Ignore literals implicitly added by the compiler, such as when using a range-for loop over a constant array.

Jul 23 2018, 6:25 PM

Jul 19 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 19 2018, 11:08 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".
./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
Jul 19 2018, 10:33 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 19 2018, 9:20 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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.

Jul 19 2018, 8:04 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Small refactoring and documentation update.

Jul 19 2018, 4:56 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 19 2018, 3:00 PM

Jul 18 2018

0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 18 2018, 7:18 PM
0x8000-0000 added a comment to D45444: [clang-tidy] implement new check for const-correctness.

Any plans for fix-its that will add the suggested 'const' qualifiers?

Jul 18 2018, 7:16 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 18 2018, 7:15 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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 18 2018, 5:13 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 18 2018, 5:06 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 18 2018, 4:48 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 18 2018, 3:08 PM

Jul 17 2018

0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 17 2018, 6:12 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 17 2018, 6:06 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 17 2018, 5:13 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "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 17 2018, 4:27 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

@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!

Jul 17 2018, 3:43 AM
0x8000-0000 added a comment to D45444: [clang-tidy] implement new check for const-correctness.

@0x8000-0000 are you interested in working on the check? If i recall correctly the branch for 7.0 will happen on 1. august. That would be the timewindow in which it makes sense to give you the patch. I will have time in september following, but not right now.
I try to commit 2 bug fixes in this time, because they are almost done already.

Jul 17 2018, 3:41 AM

Jul 14 2018

0x8000-0000 added a comment to D45444: [clang-tidy] implement new check for const-correctness.

Time :/

Jul 14 2018, 11:59 AM

Jul 13 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Accept magic values arbitrarily deep in a constant initialization

Jul 13 2018, 10:12 PM

Jul 11 2018

0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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

Jul 11 2018, 5:06 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Fix typo

Jul 11 2018, 5:06 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

Thank you for your review @Eugene.Zelenko

Jul 11 2018, 4:41 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Update documentation to clean up formatting

Jul 11 2018, 4:40 PM
0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Updated examples code and added several references

Jul 11 2018, 4:01 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 11 2018, 3:23 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

C++ Core Guidelines contains ES.45: Avoid "magic constants"; use symbolic constants, so I think check should be moved into cppcoreguidelines module.

Jul 11 2018, 3:19 PM

Jul 10 2018

0x8000-0000 updated the diff for D49114: [clang-tidy] Add a check for "magic numbers".

Incorporate review comments. Add support for floating point detection.

Jul 10 2018, 7:24 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 10 2018, 6:42 PM
0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 10 2018, 4:30 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

I think some of the logic you have in your check code could be done via matchers. That is usually better for performance, because you analyze less code.

Jul 10 2018, 3:54 AM

Jul 9 2018

0x8000-0000 added inline comments to D49114: [clang-tidy] Add a check for "magic numbers".
Jul 9 2018, 8:08 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

The cult of "no magic numbers" is horrible and we should be trying to deprogram its adherents, not create a whole new generation of them. I would be happy if this clang-tidy patch were quickly abandoned. But, it's just a clang-tidy check — it's easy for people who don't want it to ignore its existence — so I'll just plan to be in that group of people.

Jul 9 2018, 8:05 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

I suspect that the check will be very noisy for powers of 2 and 10 that are used as multiplicands. You might wish to exclude those.

Jul 9 2018, 6:51 PM
0x8000-0000 added a comment to D49114: [clang-tidy] Add a check for "magic numbers".

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.

Jul 9 2018, 6:36 PM
0x8000-0000 created D49114: [clang-tidy] Add a check for "magic numbers".
Jul 9 2018, 5:44 PM