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