aaron.ballman added a comment. In https://reviews.llvm.org/D49114#1179652, @0x8000-0000 wrote:
> Top 40 magic numbers in https://github.com/qt/qtbase > > 4859 2 > 2901 3 > 1855 4 > 985 5 > 968 8 > 605 6 > 600 7 > 439 16 > 432 10 > 363 > 356 32 > 241 1.0f > 217 12 > 209 255 > 207 100 > 205 9 > 205 20 > 204 50 > 177 0.5 > 174 15 > 162 0x2 > 144 24 > 140 0x80 > 135 11 > 127 256 > 113 14 > 110 0xff > 101 1.0 > 99 64 > 99 200 > 96 13 > 86 30 > 84 1000 > 68 18 > 66 150 > 62 127 > 62 0xFF > 58 19 > 58 0.05f > 57 128 > > > Top 40 floating point magic numbers in https://github.com/qt/qtbase > > 241 1.0f > 177 0.5 > 101 1.0 > 58 0.05f > 44 2.0 > 42 0.5f > 31 10.0 > 28 30.0 > 24 20.0 > 22 60.0 > 20 100.0 > 19 0.8 > 19 0.25 > 17 0.2 > 16 1000.0 > 14 1.224744871 > 14 100. > 13 25.0 > 13 0.1 > 12 90.0 > 12 40.0 > 12 0.707106781 > 12 0.30 > 12 0.20 > 11 80.0 > 11 6.0 > 11 50.0 > 11 2.0f > 11 0.75 > 11 0.66f > 11 0.1f > 10 6.28 > 10 5.0 > 10 4.0 > 10 1.414213562 > 9 360.0 > 9 25.4 > 9 2.54 > 8 70.0 > 8 55.0 > > > Top 40 magic numbers in https://github.com/facebook/rocksdb > > 2131 2 > 896 3 > 859 4 > 858 10 > 685 100 > 678 1024 > 600 8 > 445 5 > 323 1000 > 244 20 > 231 301 > 227 200 > 223 6 > 209 16 > 189 7 > 154 10000 > 131 1000000 > 119 100000 > 111 30 > 105 256 > 104 32 > 103 5U > 103 50 > 94 128 > 91 64 > 89 60 > 88 3U > 85 2U > 84 500 > 72 4U > 67 9 > 65 300 > 63 13 > 59 0xff > 57 6U > 52 4096 > 52 24 > 52 12 > 51 600 > 50 10U > > > Top 40 floating point numbers in rocksdb: > > 37 100.0 > 30 1.0 > 27 0.5 > 24 0.001 > 12 1048576.0 > 12 0.25 > 11 1.1 > 8 50.0 > 8 1.5 > 8 10000.0 > 5 .3 > 5 .1 > 5 0.8 > 4 99.99 > 4 99.9 > 4 20000.0 > 4 1.048576 > 4 100.0f > 4 0.9 > 4 0.75 > 4 0.69 > 4 0.02 > 4 0.00001 > 3 1000000.0 > 3 0.4 > 3 0.1 > 2 0.7 > 2 0.6 > 2 0.45 > 1 8.0 > 1 5.6 > 1 40.00002 > 1 40.00001 > 1 3.25 > 1 2.0 > 1 2. > 1 116.00002 > 1 116.00001 > 1 110.5e-4 > 1 1024.0 > Awesome, thank you for this! Based on this, I think the integer list should also include 2, 3, and 4 as defaults -- those show up a lot more than I'd have expected. As for floating-point values, 1.0 certainly jumps out at me, but none of the rest seem particularly common. What do you think? ================ 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: > > > > 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()`? 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
