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());
+
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > 0x8000-0000 wrote:
> > > 0x8000-0000 wrote:
> > > > 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` ;(
> > > Option 1 doesn't work, either:
> > > ```
> > > llvm::APFloat DoubleFloatValue(llvm::APFloat::IEEEdouble());
> > >
> > >
> > > if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble())
> > >
> > >
> > > DoubleFloatValue = FloatValue;
> > >
> > >
> > > else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEsingle())
> > > {
> > >
> > > const float Value = FloatValue.convertToFloat();
> > >
> > >
> > > DoubleFloatValue = llvm::APFloat(static_cast<double>(Value));
> > >
> > >
> > > }
> > >
> > >
> > >
> > >
> > >
> > > return std::binary_search(IgnoredFloatingPointValues.begin(),
> > >
> > >
> > > IgnoredFloatingPointValues.end(),
> > >
> > >
> > > DoubleFloatValue, compareAPFloats);
> > > ```
> > > has problems with floating point values that are not integers.
> > >
> > > Unless somebody knows how to convert APFloats with any semantics into
> > > APFloats with double semantics, I'm stuck ;(
> > > There is no
> > > `APFloat convertToOtherSemantics(const fltSemantics &) const;`
> > > method.
> > ```
> > opStatus convert(const fltSemantics &ToSemantics, roundingMode RM,
> > bool *losesInfo);
> > ```
> > This function does exactly that?
> > `TargetInfo::getDoubleFormat()` is not accessible directly from
> > `ClangTidyContext` or from `MatchFinder`
>
> `MatchFinder` has an `ASTContext` object on which you can call
> `ASTContext::getTargetInfo()`, I believe.
Yes, it does! Thank you!
================
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:
> > aaron.ballman wrote:
> > > 0x8000-0000 wrote:
> > > > 0x8000-0000 wrote:
> > > > > 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`
> > > > > ;(
> > > > Option 1 doesn't work, either:
> > > > ```
> > > > llvm::APFloat DoubleFloatValue(llvm::APFloat::IEEEdouble());
> > > >
> > > >
> > > > if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble())
> > > >
> > > >
> > > > DoubleFloatValue = FloatValue;
> > > >
> > > >
> > > > else if (&FloatValue.getSemantics() ==
> > > > &llvm::APFloat::IEEEsingle()) {
> > > >
> > > >
> > > > const float Value = FloatValue.convertToFloat();
> > > >
> > > >
> > > > DoubleFloatValue = llvm::APFloat(static_cast<double>(Value));
> > > >
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > return std::binary_search(IgnoredFloatingPointValues.begin(),
> > > >
> > > >
> > > > IgnoredFloatingPointValues.end(),
> > > >
> > > >
> > > > DoubleFloatValue, compareAPFloats);
> > > > ```
> > > > has problems with floating point values that are not integers.
> > > >
> > > > Unless somebody knows how to convert APFloats with any semantics into
> > > > APFloats with double semantics, I'm stuck ;(
> > > > There is no
> > > > `APFloat convertToOtherSemantics(const fltSemantics &) const;`
> > > > method.
> > > ```
> > > opStatus convert(const fltSemantics &ToSemantics, roundingMode RM,
> > > bool *losesInfo);
> > > ```
> > > This function does exactly that?
> > > `TargetInfo::getDoubleFormat()` is not accessible directly from
> > > `ClangTidyContext` or from `MatchFinder`
> >
> > `MatchFinder` has an `ASTContext` object on which you can call
> > `ASTContext::getTargetInfo()`, I believe.
> Yes, it does! Thank you!
Looking at the code, it declares a structure for MatchResult which indeed
contains an ASTContext. But MatchFinder does not hold a reference to it, it is
passed into it from a MatchVisitor.
What we would need is to parse the configuration before we start matching
(either in the constructor or in `::registerMatchers` - and there is no visitor
available then.
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
