[PATCH] D71686: Fix false positive in magic number checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D71686#1794377 , @0x8000- wrote:

> @aaron.ballman updated as suggested; please commit/integrate when you have a 
> moment. Thank you!


Happy to do so, thank you for the fix! I've commit in 
c16b3ec597d277b5a7397db308f8ec730f3330a3 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@aaron.ballman updated as suggested; please commit/integrate when you have a 
moment. Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked 2 inline comments as done.
0x8000- added a comment.

In D71686#1794366 , @aaron.ballman 
wrote:

> In D71686#1794360 , @0x8000- 
> wrote:
>
> > In D71686#1794330 , @aaron.ballman 
> > wrote:
> >
> > > In D71686#1794053 , @0x8000- 
> > > wrote:
> > >
> > > > My take: this change fixes a user-reported bug, and does not cause any 
> > > > known regressions. I think we should integrate this.
> > >
> > >
> > > I sort of wonder whether we want to document this as a blessed approach 
> > > to silencing the warning. I'm not certain if it's too obtuse or not, but 
> > > I notice the check has no documented ways to silence the diagnostic aside 
> > > from using the correct kind of magic number or adding it to a list of 
> > > excluded magic numbers.
> >
> >
> > You mean Hyrum's Law  is not sufficient?
> >
> > The check can be silenced with the regular NOLINT, or with defining and 
> > using a constant/enum. Using this "backdoor" way seems even more cumbersome 
> > and confusing than the NOLINT. At least with NOLINT it is clear what you're 
> > doing, and somebody else can grep for it and fix it if it is appropriate.
>
>
> My concern is that `NOLINT` is insufficient. Consider: `foo(12, 42, 18);` 
> where the `42` is not desired to be warned about due to domain-specific 
> knowledge but the `12` and `18` are.
>
> However, I am not convinced that you're wrong either -- casting is 
> cumbersome. But it's also a somewhat well-used workaround to silence warnings 
> (casting to void silencing unused value warnings being a common example).
>
> For right now, we can leave it as a bug -- we can decide to bless the 
> approach later.


Fair enough; I'm aiming for good precision and recall by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 235060.
0x8000- added a comment.

Minor comment fixes; capitalization, full stop.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
@@ -183,7 +183,7 @@
 
 const geometry::Rectangle mandelbrotCanvas{geometry::Point{-2.5, -1}, geometry::Dimension{3.5, 2}};
 
-// Simulate the macro magic in Google Test internal headers
+// Simulate the macro magic in Google Test internal headers.
 class AssertionHelper {
 public:
   AssertionHelper(const char *Message, int LineNumber) : Message(Message), LineNumber(LineNumber) {}
@@ -201,7 +201,7 @@
   ASSERTION_HELPER("here and now");
 }
 
-// prove that integer literals introduced by the compiler are accepted silently
+// Prove that integer literals introduced by the compiler are accepted silently.
 extern int ConsumeString(const char *Input);
 
 const char *SomeStrings[] = {"alpha", "beta", "gamma"};
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// Prove that using enumerations values don't produce warnings.
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t --
+// XFAIL: *
+
+int ProcessSomething(int input);
+
+int DoWork()
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+return 0;
+
+   return 0;
+}
+
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -125,8 +125,20 @@
 if (isUsedToInitializeAConstant(Result, Parent))
   return true;
 
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get() &&
+llvm::any_of(
+Result.Context->getParents(Parent),
+[](const DynTypedNode &GrandParent) {
+  return GrandParent.get() !=
+ nullptr;
+}))
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
 if (Parent.get())
   return true;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:2
+// RUN: %check_clang_tidy %s readability-magic-numbers %t
+// RUN: --
+// XFAIL: *

You might as well move this onto the previous line; it just looks strange here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp:219
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {

prove -> Prove

and add a full stop to the end of the comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71686#1794360 , @0x8000- wrote:

> In D71686#1794330 , @aaron.ballman 
> wrote:
>
> > In D71686#1794053 , @0x8000- 
> > wrote:
> >
> > > My take: this change fixes a user-reported bug, and does not cause any 
> > > known regressions. I think we should integrate this.
> >
> >
> > I sort of wonder whether we want to document this as a blessed approach to 
> > silencing the warning. I'm not certain if it's too obtuse or not, but I 
> > notice the check has no documented ways to silence the diagnostic aside 
> > from using the correct kind of magic number or adding it to a list of 
> > excluded magic numbers.
>
>
> You mean Hyrum's Law  is not sufficient?
>
> The check can be silenced with the regular NOLINT, or with defining and using 
> a constant/enum. Using this "backdoor" way seems even more cumbersome and 
> confusing than the NOLINT. At least with NOLINT it is clear what you're 
> doing, and somebody else can grep for it and fix it if it is appropriate.


My concern is that `NOLINT` is insufficient. Consider: `foo(12, 42, 18);` where 
the `42


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D71686#1794330 , @aaron.ballman 
wrote:

> In D71686#1794053 , @0x8000- 
> wrote:
>
> > My take: this change fixes a user-reported bug, and does not cause any 
> > known regressions. I think we should integrate this.
>
>
> I sort of wonder whether we want to document this as a blessed approach to 
> silencing the warning. I'm not certain if it's too obtuse or not, but I 
> notice the check has no documented ways to silence the diagnostic aside from 
> using the correct kind of magic number or adding it to a list of excluded 
> magic numbers.


You mean Hyrum's Law  is not sufficient?

The check can be silenced with the regular NOLINT, or with defining and using a 
constant/enum. Using this "backdoor" way seems even more cumbersome and 
confusing than the NOLINT. At least with NOLINT it is clear what you're doing, 
and somebody else can grep for it and fix it if it is appropriate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71686#1794053 , @0x8000- wrote:

> My take: this change fixes a user-reported bug, and does not cause any known 
> regressions. I think we should integrate this.


I sort of wonder whether we want to document this as a blessed approach to 
silencing the warning. I'm not certain if it's too obtuse or not, but I notice 
the check has no documented ways to silence the diagnostic aside from using the 
correct kind of magic number or adding it to a list of excluded magic numbers.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

My take: this change fixes a user-reported bug, and does not cause any known 
regressions. I think we should integrate this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:9
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]

This test fails even with clang-tidy-9.


```
$ clang-tidy-9 --checks="-*,readability-magic-numbers" 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp

/home/florin/tools/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:9:35:
 warning: 10 is a magic number; consider replacing it with a named constant 
[readability-magic-numbers]
  if (((int)4) > ProcessSomething(10))
  ^
```

Nothing about the "4" here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234998.
0x8000- added a comment.

Add a test file for expected failures


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
 
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t
+// RUN: --
+// XFAIL: *
+
+int ProcessSomething(int input);
+
+int DoWork()
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
+return 0;
+
+   return 0;
+}
+
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -125,8 +125,20 @@
 if (isUsedToInitializeAConstant(Result, Parent))
   return true;
 
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get() &&
+llvm::any_of(
+Result.Context->getParents(Parent),
+[](const DynTypedNode &GrandParent) {
+  return GrandParent.get() !=
+ nullptr;
+}))
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
 if (Parent.get())
   return true;
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+templa

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+// expanded class enumeration value.
+if (Parent.get())
+  return true;

0x8000- wrote:
> aaron.ballman wrote:
> > So will this no longer treat *any* C-style cast as being a magic number? 
> > e.g., `int i; i = (int)12;`
> Sadly, it would seem so.
> 
> if (ValueArray[(int)4] > ValueArray[1]) produces the following AST:
> 
> ```
> |   | | | `-ArraySubscriptExpr 0x11545f0  'int' lvalue
> |   | | |   |-ImplicitCastExpr 0x11545d8  'int *' 
> |   | | |   | `-DeclRefExpr 0x1154558  'int [5]' lvalue Var 0x1153cb0 
> 'ValueArray' 'int [5]'
>
> |   | | |   `-CStyleCastExpr 0x11545b0  'int' 
> |   | | | `-IntegerLiteral 0x1154578  'int' 4
> ```
> 
> While the test case we're trying to avoid is:
> ```
> `-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
>   |-TemplateArgument expr
>   | `-ConstantExpr 0x1169dc0  'Letter' 9
>   |   `-SubstNonTypeTemplateParmExpr 0x1169da0  'Letter'
>   | `-CStyleCastExpr 0x1169d78  'Letter' 
>   |   `-IntegerLiteral 0x1169d48  'unsigned int' 9
>   `-RecordType 0x1169ec0 'holder'
> `-ClassTemplateSpecialization 0x1169de0 'holder'
> ```
> 
> Now, the question is - do we care? How many people use a C cast with a bare 
> integer?
> 
> I suppose I can add a check for the grandparent node to be 
> SubstNonTypeTemplateParmExpr, in order to filter this out?
An interesting fact is that the C-style cast does not produce a warning now 
anyway (even before applying this change.) and I don't have any idea why.

Adding these lines does not create a test failure:

```
+  79   if (((int)4) > IntSquarer[10])  
   
+  80 return 0;  
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+// expanded class enumeration value.
+if (Parent.get())
+  return true;

aaron.ballman wrote:
> So will this no longer treat *any* C-style cast as being a magic number? 
> e.g., `int i; i = (int)12;`
Sadly, it would seem so.

if (ValueArray[(int)4] > ValueArray[1]) produces the following AST:

```
|   | | | `-ArraySubscriptExpr 0x11545f0  'int' lvalue
|   | | |   |-ImplicitCastExpr 0x11545d8  'int *' 
|   | | |   | `-DeclRefExpr 0x1154558  'int [5]' lvalue Var 0x1153cb0 
'ValueArray' 'int [5]'  
 
|   | | |   `-CStyleCastExpr 0x11545b0  'int' 
|   | | | `-IntegerLiteral 0x1154578  'int' 4
```

While the test case we're trying to avoid is:
```
`-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
  |-TemplateArgument expr
  | `-ConstantExpr 0x1169dc0  'Letter' 9
  |   `-SubstNonTypeTemplateParmExpr 0x1169da0  'Letter'
  | `-CStyleCastExpr 0x1169d78  'Letter' 
  |   `-IntegerLiteral 0x1169d48  'unsigned int' 9
  `-RecordType 0x1169ec0 'holder'
`-ClassTemplateSpecialization 0x1169de0 'holder'
```

Now, the question is - do we care? How many people use a C cast with a bare 
integer?

I suppose I can add a check for the grandparent node to be 
SubstNonTypeTemplateParmExpr, in order to filter this out?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+// expanded class enumeration value.
+if (Parent.get())
+  return true;

So will this no longer treat *any* C-style cast as being a magic number? e.g., 
`int i; i = (int)12;`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234819.
0x8000- added a comment.

Fix typo.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
const Expr &ExprResult) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [&Result](const DynTypedNode &Parent) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [&Result](const DynTypedNode &Parent) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- 

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234818.
0x8000- edited the summary of this revision.
0x8000- added a comment.

Update release notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the syntethically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
const Expr &ExprResult) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [&Result](const DynTypedNode &Parent) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [&Result](const DynTypedNode &Parent) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===

[PATCH] D71686: Fix false positive in magic number checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention fix in Release Notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:127
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.

127-130 are the new lines. The rest were moved about by clang-format. Please 
let me know if I should revert the format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71686/new/

https://reviews.llvm.org/D71686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- created this revision.
0x8000- added a reviewer: aaron.ballman.
0x8000- added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix false positive in magic number checker

https://bugs.llvm.org/show_bug.cgi?id=40640: 
cppcoreguidelines-avoid-magic-numbers should not warn about enum class


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings (code by Pavel 
Kryukov)
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
const Expr &ExprResult) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [&Result](const DynTypedNode &Parent) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [&Result](const DynTypedNode &Parent) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings (code by Pavel Kryukov)
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbers