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

[Florin Iucha via Phabricator via cfe-commits]

0x8000-0000 marked 4 inline comments as done.
0x8000-0000 added a comment.

See inline comments. Basically we need two arrays because APFloats of different semantics don't compare well, and even if we coerce them, they sometimes are not equal.



================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
----------------
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?


================
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());
+
----------------
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.


================
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()) {
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114