[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Aug 5 07:32:04 PDT 2018


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





More information about the cfe-commits mailing list