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

[Florin Iucha via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+                        // expanded class enumeration value.
+                        if (Parent.get<CStyleCastExpr>())
+                          return true;
----------------
0x8000-0000 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 <col:7, col:24> 'int' lvalue
> |   | | |   |-ImplicitCastExpr 0x11545d8 <col:7> 'int *' <ArrayToPointerDecay>
> |   | | |   | `-DeclRefExpr 0x1154558 <col:7> 'int [5]' lvalue Var 0x1153cb0 'ValueArray' 'int [5]'                                                           
> |   | | |   `-CStyleCastExpr 0x11545b0 <col:18, col:23> 'int' <NoOp>
> |   | | |     `-IntegerLiteral 0x1154578 <col:23> 'int' 4
> ```
> 
> While the test case we're trying to avoid is:
> ```
>     `-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
>       |-TemplateArgument expr
>       | `-ConstantExpr 0x1169dc0 <col:59> 'Letter' 9
>       |   `-SubstNonTypeTemplateParmExpr 0x1169da0 <col:59> 'Letter'
>       |     `-CStyleCastExpr 0x1169d78 <col:59> 'Letter' <IntegralCast>
>       |       `-IntegerLiteral 0x1169d48 <col:59> 'unsigned int' 9
>       `-RecordType 0x1169ec0 'holder<Letter::J>'
>         `-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