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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > I would still like to see some data on common floating-point literal values used in large open source project so that we can see what sensible values should be in this list.
> What value would that bring? The ideal target is that there are no magic values - no guideline that I have seen makes exception for 3.141 or 9.81. Each project is special based on how they evolved, and they need to decide for themselves what is worth cleaning vs what can be swept under the rug for now. Why would we lend authority to any particular floating point value?
Because that's too high of a high false positive rate for an acceptable clang-tidy check. As mentioned before, there are literally hundreds of unnameable floating-point literals in LLVM alone where the value is 1.0 or 2.0. Having statistical data to pick sensible defaults for this list is valuable in that it lowers the false positive rate. If the user dislikes the default list for some reason (because for their project, maybe 2.0 is a supremely nameable literal value), they can pick a different set of defaults.

Right now, I'm operating off an assumption that most floating-point literals that should not be named are going to be whole numbers that are precisely represented in all floating-point semantic models. This data will tell us if that assumption is wrong, and if the assumption is wrong, we might want to go with separate lists like you've done.


================
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:
> > 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.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:128-139
+  if (FloatValue.isZero())
+    return true;
+  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()) {
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > Here is where I would compare `FloatValue` against everything in the `IgnoredFloatingPointValues` array using `APFloat::compare()`. However, you need the floats to have the same semantics for that to work, so you'd have to convert `FloatValue` using `APFloat::convert()` to whatever the target floating-point format is.
> If you do that, 3.14f will not be equal to 3.14 . You need two arrays.
The point to a named constants check is to give names to literal values that should have names because there's something special about them. No one wants to name 1 because they're incrementing by one, or 2.0 because they're cutting a value in half. So the thrust of this check should be to make it hard for users to have literals without names while making it easy to provide exceptions for truly unnameable values because they don't have semantic meaning. I am not worried that `3.14` is not equal to `3.14f` because numbers which require precision most likely *should* be given named constants because there's most likely *something* special about that value.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114