aaron.ballman added inline comments.
================ Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57 +const char DefaultIgnoredIntegerValues[] = "0;1;"; +const char DefaultIgnoredFloatingPointValues[] = "0.0;"; + ---------------- 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). 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
