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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
