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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.
----------------
0x8000-0000 wrote:
> 0x8000-0000 wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > 0x8000-0000 wrote:
> > > > > aaron.ballman wrote:
> > > > > > I am curious to know how true this is. You got some data for integer values and reported it, but I'm wondering if you've tried the same experiment with floating-point numbers?
> > > > > The problem with the floating point numbers as text is: they need to be parsed both from the configuration and from the source code _then_ compared. What is an acceptable epsilon? I don't know. Is the same epsilon acceptable on all source code? I don't know.
> > > > Yeah, I'm not too worried about the situations in which the epsilon matters. I'm more worried that we'll see a lot of 1.0, 2.0 floating-point literals where the floating-point value is a nice, round, easy-to-represent number but users have no way to disable this diagnostic short of `const float Two = 2.0f;`
> > > Random thought: the types that are ignored should/could be configurable, i.e. there should be a switch
> > > whether or not to complain about floats.
> > Even though they might be nice and round... they should mean _something_ other than 'Two'.
> > 
> > The thing is... magic integers are used as buffer sizes, or to map things that are discrete in nature - number of legs of a typical mammal for instance. Not sure what magic numbers exist in nature besides pi and e and some fundamental physical constants )Avogadro's number, etc). But even there, it is better to use a symbolic constant.
> Actually that is a _great_ idea, thank you!
> The thing is... magic integers are used as buffer sizes, or to map things that are discrete in nature - number of legs of a typical mammal for instance. Not sure what magic numbers exist in nature besides pi and e and some fundamental physical constants )Avogadro's number, etc). But even there, it is better to use a symbolic constant.

That's my point -- I think there's a lot of uses of round floating-point values that are not magical numbers, they're sensible constants. Looking at LLVM's code base shows a *lot* of 1.0 and 2.0 values (hundreds of instances from a quick text-based search). No one should be forced to turn those into named constants. However, I've seen code using `1.02` and `.98` in places -- those seem like sensible things to make named constants because the values have semantically interesting meaning to the surrounding code.

> Random thought: the types that are ignored should/could be configurable, i.e. there should be a switch
whether or not to complain about floats.

I think this would be a useful option, for sure (I used to work at a place that did a ton of floating-point math that would benefit from the integer side of this check but could never use the floating-point side of it). However, the presence of such an option doesn't give us a pass on coming up with a data-driven list of default values to ignore for the floating-point side. If we don't want to make that list configurable, I think that's something we can discuss (I think I'm fine with not making it a user-facing configuration option). But I think that `0.0` is insufficient.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114