aaron.ballman added a comment.
I think this is close. If @alexfh doesn't chime in on the open question in the
next few days, I would say the check is ready to go in and we can address the
open question in follow-up commits.
================
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:
> > > 0x8000-0000 wrote:
> > > > 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.
> > > So functionally it works, but it suffers from the same problem as doing
> > > the conversion by hand using the
> > > ```static_cast<double>(FloatValue.convertToFloat())```; the resulting
> > > value for 3.14f does not match the existing value for 3.14 present in the
> > > IgnoredFloatingPointValues array.
> > Hmm, that's certainly annoying.
> >
> > @alexfh -- I think we should have access to the ASTContext object from
> > within the ClangTidyContext passed in when registering the checks. We
> > already have `ClangTidyContext::setASTContext()` that gets called before
> > `createChecks()` in `ClangTidyASTConsumerFactory::CreateASTConsumer()`; can
> > you think of a reason we should not persist that value in the
> > `ClangTidyContext`? If there is a good reason to not persist it, perhaps it
> > would be a reasonable argument to be passed to each check's constructor (by
> > threading it through `createChecks()`?
> ```
> bool MagicNumbersCheck::isIgnoredValue(const FloatingLiteral *Literal) const {
> llvm::APFloat FloatValue = Literal->getValue();
> if (FloatValue.isZero())
> return true;
> else {
> bool LosesInfo = true;
> const llvm::APFloat::opStatus ConvertStatus = FloatValue.convert(
> llvm::APFloat::IEEEdouble(), DefaultRoundingMode, &LosesInfo);
>
> if ((ConvertStatus == llvm::APFloat::opOK) ||
> (ConvertStatus == llvm::APFloat::opInexact))
> return std::binary_search(IgnoredFloatingPointValues.begin(),
> IgnoredFloatingPointValues.end(), FloatValue,
> compareAPFloats);
> else
> return false;
> }
> }
> ```
>
> "3.14f" FloatValue converts cleanly... to something that is not present in
> the IgnoredFloatingPointValues set.
@alexfh: pinging on this question.
================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:35-38
+ if (AsDecl->isImplicit())
+ return true;
+
+ return false;
----------------
This can be reduced to `return AsDecl->isImplicit();`
================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:134
+ else {
+ if (IgnorePowersOf2IntegerValues && isPowerOfTwo(Value))
+ return true;
----------------
Drop the custom function and instead use `IntValue.isPowerOf2()` instead.
Also, no `else` after a `return`.
================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:146-154
+ else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEsingle()) {
+ const float Value = FloatValue.convertToFloat();
+ return std::binary_search(IgnoredFloatingPointValues.begin(),
+ IgnoredFloatingPointValues.end(), Value);
+ } else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble()) {
+ const double Value = FloatValue.convertToDouble();
+ return std::binary_search(IgnoredDoublePointValues.begin(),
----------------
No `else` after `return`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
