lebedev.ri 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. ---------------- aaron.ballman wrote: > 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. Yep. I didn't mean for that flag to be a replacement for the ignore-list for fp constants. 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
