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

Sun Jul 29 08:56:05 PDT 2018

0x8000-0000 added inline comments.

================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
----------------
aaron.ballman wrote:
> 0x8000-0000 wrote:
> > aaron.ballman wrote:
> > > 0x8000-0000 wrote:
> > > > aaron.ballman wrote:
> > > > > I would still like to see some data on common floating-point literal values used in large open source project so that we can see what sensible values should be in this list.
> > > > What value would that bring? The ideal target is that there are no magic values - no guideline that I have seen makes exception for 3.141 or 9.81. Each project is special based on how they evolved, and they need to decide for themselves what is worth cleaning vs what can be swept under the rug for now. Why would we lend authority to any particular floating point value?
> > > Because that's too high of a high false positive rate for an acceptable clang-tidy check. As mentioned before, there are literally hundreds of unnameable floating-point literals in LLVM alone where the value is 1.0 or 2.0. Having statistical data to pick sensible defaults for this list is valuable in that it lowers the false positive rate. If the user dislikes the default list for some reason (because for their project, maybe 2.0 is a supremely nameable literal value), they can pick a different set of defaults.
> > > 
> > > Right now, I'm operating off an assumption that most floating-point literals that should not be named are going to be whole numbers that are precisely represented in all floating-point semantic models. This data will tell us if that assumption is wrong, and if the assumption is wrong, we might want to go with separate lists like you've done.
> > Here are the results with the check as-is, run on the llvm code base as of last night:
> > 
> > top-40
> > ```
> >   10435 2
> >    5543 4
> >    4629 8
> >    3271 3
> >    2702 16
> >    1876 32
> >    1324 64
> >    1309 10
> >    1207 5
> >    1116 128
> >     966 6
> >     733 7
> >     575 256
> >     421 20
> >     406 12
> >     339 9
> >     331 1024
> >     311 100
> >     281 42
> >     253 11
> >     226 15
> >     189 40
> >     172 24
> >     169 0xff
> >     168 13
> >     168 0x80
> >     166 512
> >     137 1.0
> >     133 14
> >     132 31
> >     129 0xDEADBEEF
> >     120 18
> >     120 17
> >     120 1000
> >     115 4096
> >     100 30
> >      94 60
> >      94 0x1234
> >      89 0x20
> >      86 0xFF
> > ```
> > 
> > 1.0 is in position 28 with 137 occurrences
> > 2.0 is in position 93 with 27 occurrences
> > 100.0 is in position 96 with 26 occurences
> > 1.0f is in position 182 with 11 occurences
> > 
> > we also have 2.0e0 four times :)
> > 
> > This data suggests that there would be value in a IgnorePowerOf2IntegerLiterals option.
> Any chance you can hack the check run on LLVM where it doesn't report any integer values (I'm curious about the top ten or so for floating-point values)? Additionally, it'd be great to get similar numbers from some other projects, like https://github.com/qt just to see how it compares (I've used `bear` to get compile_commands.json file out of Qt before, so I would guess that would work here).
No need for the hack, I just grep for dot in my report:

```
    137 1.0
     27 2.0
     26 100.0
     20 0.5
     11 1.0f
      8 1.02
      7 3.0
      7 1.98
      7 1.5
      6 .5
      6 1.1
      5 0.01
      4 89.0f
      4 4294967296.f
      4 3.14159
      4 2.0e0
      4 10.0
      3 88.0f
      3 255.0
      3 127.0f
      3 12345.0f
      3 123.45
      3 0.2f
      3 0.25
      3 0.1f
      3 0.1
      2 80.0
      2 710.0f
      2 710.0
      2 709.0f
      2 6.0
      2 3.14f
      2 3.14
      2 2.5
      2 2349214918.58107
      2 149.0f
      2 14.5f
      2 1.17549435e-38F
      2 11357.0f
      2 11356.0f
      2 103.0f
      2 0.99
      2 0.9
      2 0.01f
      1 745.0f
      1 745.0
      1 7.1653228759765625e2f
      1 709.0
      1 7.08687663657301358331704585496e-268
      1 6.62E-34
      1 64.0f
      1 6.02E23
      1 4950.0f
      1 4932.0f
      1 45.0f
      1 42.42
      1 4.2
      1 4.0
      1 38.0f
      1 3.7
      1 323.0f
      1 32.0f
      1 32.0
      1 3.1415
      1 308.0f
      1 2.718
      1 2.7
      1 225.0f
      1 21.67
      1 2.0f
      1 2.088
      1 .17532f
      1 16445.0f
      1 1.616f
      1 128.0f
      1 12346.0f
      1 12.0f
      1 1.2
      1 1.1f
      1 11399.0f
      1 11383.0f
      1 1.0L
      1 1.0E+9
      1 1.0E+6
      1 1.0e-5f
      1 1.0E+12
      1 1074.0f
      1 1074.0
      1 1023.0f
      1 1023.0
      1 1.01F
      1 1.01
      1 10.0f
      1 100.
      1 0.999999
      1 0.8f
      1 .08215f
      1 0.7
      1 0.6
      1 0.5F
      1 0.5f
      1 0.59
      1 0.4f
      1 0.2
      1 0.06
      1 0.02
      1 0.005
```

I'll check out Qt shortly.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114