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