[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 29 11:18:14 PDT 2018
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:
> 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?
================
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:
> 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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
More information about the cfe-commits
mailing list