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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+    llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+    FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+    IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > 0x8000-0000 wrote:
> > > aaron.ballman wrote:
> > > > This is where I would construct an `APFloat` object from the string given. As for the semantics to be used, I would recommend getting it from `TargetInfo::getDoubleFormat()` on the belief that we aren't going to care about precision (explained in the documentation).
> > > Here is the problem I tried to explain last night but perhaps I wasn't clear enough.
> > > 
> > > When we parse the input list from strings, we have to commit to one floating point value "semantic" - in our case single or double precision.
> > > 
> > > When we encounter the value in the source code and it is captured by a matcher, it comes as either one of those values.
> > > 
> > > Floats with different semantics can't be directly compared - so we have to maintain two distinct arrays.
> > > 
> > > If we do that, rather than store APFloats and sort/compare them with awkward lambdas, we might as well just use the native float/double and be done with it more cleanly.
> > >When we encounter the value in the source code and it is captured by a matcher, it comes as either one of those values.
> > 
> > It may also come in as long double or __float128, for instance, because there are type suffixes for that.
> > 
> > > Floats with different semantics can't be directly compared - so we have to maintain two distinct arrays.
> > 
> > Yes, floats with different semantics cannot be directly compared. That's why I said below that we should coerce the literal values.
> > 
> > > If we do that, rather than store APFloats and sort/compare them with awkward lambdas, we might as well just use the native float/double and be done with it more cleanly.
> > 
> > There are too many different floating-point semantics for this to be viable, hence why coercion is a reasonable behavior.
> Let me see if I understood it - your proposal is: store only doubles, and when a floating-point literal is encountered in code, do not use the FloatingLiteral instance, but parse it again into a double and compare exactly. If the comparison matches - ignore it.
> 
> In that case what is the value of storing APFloats with double semantics in the IgnoredValues array, instead of doubles?
> Let me see if I understood it - your proposal is: store only doubles, and when a floating-point literal is encountered in code, do not use the FloatingLiteral instance, but parse it again into a double and compare exactly. If the comparison matches - ignore it.

My proposal is to use `APFloat` as the storage and comparison medium. Read in strings from the configuration and convert them to an `APFloat` that has double semantics. Read in literals and call `FloatLiteral::getValue()` to get the `APFloat` from it, convert it to one that has double semantics as needed, then perform the comparison between those two `APFloat` objects.

> In that case what is the value of storing APFloats with double semantics in the IgnoredValues array, instead of doubles?

Mostly that it allows us to modify or extend the check for more complicated semantics in the future. Also, it's good practice to use something with consistent semantic behavior across hosts and targets (comparisons between numbers that cannot be precisely represented will at least be consistently compared across hosts when compiling for the same target).



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114