[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 : Parents) {
+
+ // the definition of const values themselves, as global or local
variables
please remove this line.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:45
+
+ // the definition of const values themselves, as global or local
variables
+ {
Please make the comments full sentences.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:46
+ // the definition of const values themselves, as global or local
variables
+ {
+const auto *AsVarDecl = Parent.get();
Every scope you introduce here should be a function, i think.
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:11
+
+ double circleArea = 3.1415926535 * radius * radius;
+
This example is good, but right now the code only checks for integer literals.
Maybe an integer example would be better?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 integer literal 2
[readability-magic-numbers]
Quuxplusone wrote:
> How come you diagnose `2 * x` but not `x + x` or `x << 1`? Do you have a
> rationale for why the former is worse than the latter?
No tool is perfect. The intent is not to unpack any craftily obfuscated code,
but to gently guide the design and thinking and reviews..
Comment at: test/clang-tidy/readability-magic-numbers.cpp:56
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 3
[readability-magic-numbers]
Quuxplusone wrote:
> Given that you're trying to diagnose certain kinds of static data arrays and
> not others, I'd be interested to see what you think ought to happen if you
> put `{3, 5}` in a context where it creates an `initializer_list` of static
> lifetime.
The principle I've followed until now is - if you're ultimately
defining/constructing a constant object - then it should be acceptable. If
you're just seeding the object and those values will change over the program's
lifetime, it is better to seed them with named constants.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 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. There are several problems with magic numbers but the worst one is when you have a few of them sprinkled through the code (4, 5, 24) and you make one change - let's say 4 needs to become a 5. Now: how many other values do you need to change? Do you change the 5 because it is 4 + 1? Do you change 24 because is it 4 * 6? Do the 4s in these sub-expressions relate to the same concept/constant/value or not? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 people who don't want it to ignore its
existence — so I'll just plan to be in that group of people.
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 integer literal 2
[readability-magic-numbers]
How come you diagnose `2 * x` but not `x + x` or `x << 1`? Do you have a
rationale for why the former is worse than the latter?
Comment at: test/clang-tidy/readability-magic-numbers.cpp:56
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 3
[readability-magic-numbers]
Given that you're trying to diagnose certain kinds of static data arrays and
not others, I'd be interested to see what you think ought to happen if you put
`{3, 5}` in a context where it creates an `initializer_list` of static lifetime.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 initialized using expressions > such as 1 << 5? Some tests might be useful in this regard. Even better point! I will implement those suggestions and update the review packet. Thank you, florin In https://reviews.llvm.org/D49114#1156835, @hfinkel wrote: > > 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 to > > 100% calculations. 2 and 10 due to logarithms, etc. Not sure where to draw > > the line there. > > 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. > > Also, what happens for enums? Especially when initialized using expressions > such as 1 << 5? Some tests might be useful in this regard. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 to > 100% calculations. 2 and 10 due to logarithms, etc. Not sure where to draw > the line there. 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. Also, what happens for enums? Especially when initialized using expressions such as 1 << 5? Some tests might be useful in this regard. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 sure where to draw the line there. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: Add a clang-tidy check for "magic numbers"
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 example:
double circleArea = 3.1415926535 * radius * radius;
Good example:
double circleArea = M_PI * radius * radius;
This version detects and report integers only. If there is interest of merging
the tool I can add the functionality for floats as well.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MagicNumbersCheck.cpp
clang-tidy/readability/MagicNumbersCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-magic-numbers.rst
test/clang-tidy/readability-magic-numbers.cpp
Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t
+
+template
+struct ValueBucket {
+ T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 5 [readability-magic-numbers]
+
+int IntSquarer(int param) {
+ return param * param;
+}
+
+void BuggyFunction() {
+ int BadLocalInt = 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 6 [readability-magic-numbers]
+
+ (void)IntSquarer(7);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 7 [readability-magic-numbers]
+
+ int LocalArray[8];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: magic number integer literal 8 [readability-magic-numbers]
+
+ for (int ii = 0; ii < 8; ++ii)
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: magic number integer literal 8 [readability-magic-numbers]
+ {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: magic number integer literal 3 [readability-magic-numbers]
+ }
+
+ ValueBucket Bucket;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 4 [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+ TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(2), anotherConstant(val + val) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: magic number integer literal 2 [readability-magic-numbers]
+
+ int getValue() const;
+
+private:
+ int oneMember = 9;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: magic number integer literal 9 [readability-magic-numbers]
+
+ int anotherMember;
+
+ int yetAnotherMember;
+
+ const int oneConstant = 2;
+
+ const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 3 [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: magic number integer literal 5 [readability-magic-numbers]
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntContant = 42;
+
+constexpr int AlsoGoodGlobalIntContant = 42;
+
+void SolidFunction() {
+ const int GoodLocalIntContant = 43;
+
+ (void)IntSquarer(GoodLocalIntContant);
+
+ int LocalArray[INT_MACRO];
+
+ ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
Index: docs/clang-tidy/checks/readability-magic-numbers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-magic-numbers.rst
@@ -0,0 +1,15 @@
+.. title:: clang-tidy - readability-magic-numbers
+
+readability-magic-numbers
+==
+
+Detects magic numbers, integer or floating point literal that are embedded in
+code and not introduced via constants or symbols.
+
+Bad example:
+
+ double circleArea = 3.1415926535 * radius * radius;
+
+Good example:
+
+ double circleArea = M_PI * radius * radius;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -217,6 +217,7 @@
readability-identifier-naming
readability-implicit-bool-conversion
readability-inconsistent-declaration-parameter-name
+ readability-magic-numbers
readability-misleading-indentation
readability-misplaced-array-index
readability-named-parameter
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
Improvements to clang-tidy
--
+- New `readability-magic-numbers
+ `_ check
+
+ Detect usage of magic numbers, numbers that are used as literals instead of
+ introduced via constants or symbols.
+
