[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-10 Thread Jonas Toth via Phabricator via cfe-commits
JonasToth added a comment. 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. Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:44 +for (const auto &Parent : Parent

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-10 Thread Jonas Toth via Phabricator via cfe-commits
JonasToth added a comment. Related to this check: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es45-avoid-magic-constants-use-symbolic-constants Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 _

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Florin Iucha via Phabricator via cfe-commits
0x8000- added inline comments. Comment at: test/clang-tidy/readability-magic-numbers.cpp:38 +public: + TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(2), anotherConstant(val + val) {} + // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: magic number intege

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Florin Iucha via Phabricator via cfe-commits
0x8000- added a comment. In https://reviews.llvm.org/D49114#1156878, @Quuxplusone wrote: > 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

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Arthur O'Dwyer via Phabricator via cfe-commits
Quuxplusone added a comment. 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 peop

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Florin Iucha via Phabricator via cfe-commits
0x8000- added a comment. In https://reviews.llvm.org/D49114#1156835, @hfinkel wrote: > 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. Good point. > Also, what happens for enums? Especially when initiali

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Hal Finkel via Phabricator via cfe-commits
hfinkel added a comment. > This version detects and report integers only. If there is interest of > merging the tool I can add the functionality for floats as well. FWIW: I think that the FP check would be interesting. > Also I have seen coding guidelines suggesting "100" is grandfathered due t

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Florin Iucha via Phabricator via cfe-commits
0x8000- added a comment. 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. Also I have seen coding guidelines suggesting "100" is grandfathered due to 100% calculations. 2 and 10 due to logarithms, etc. Not su

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

2018-07-09 Thread Florin Iucha via Phabricator via cfe-commits
0x8000- created this revision. 0x8000- added reviewers: Wizard, aaron.ballman, alexfh, hokein. Herald added subscribers: cfe-commits, mgorny. Add a clang-tidy check for "magic numbers", integers and floating point values embedded in the code instead of using symbols or constants. Bad exa