0x8000-0000 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:
> 0x8000-0000 wrote:
> > 0x8000-0000 wrote:
> > > aaron.ballman wrote:
> > > > 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).
> > > >
> > > ok - coming right up!
> > > 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.
> >
> > This is easy.
> >
> > > Read in literals and call FloatLiteral::getValue() to get the APFloat
> > > from it,
> >
> > this is easy as well,
> >
> > > convert it to one that has double semantics as needed, then perform the
> > > comparison between those two APFloat objects.
> >
> > The conversion methods in `APFloat` only produce plain-old-data-types:
> > ```
> > double convertToDouble() const;
> >
> >
> > float convertToFloat() const;
> > ```
> >
> > There is no
> > ```
> > APFloat convertToOtherSemantics(const fltSemantics &) const;
> > ```
> > method.
> >
> > What I can do is
> > 1. convert the APFloat to float or double, depending on what the semantics
> > is; cast to double then load into an APFloat with double semantics and then
> > search into the set
> > 2. parse the textual representation of the FloatingLiteral directly into an
> > APFloat with double semantics.
> `TargetInfo::getDoubleFormat()` is not accessible directly from
> `ClangTidyContext` or from `MatchFinder`
2. doesn't quite work; `APFloat.convertFromString` chokes on `3.14f` ;(
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
