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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I think it's getting closer! I'd use a slightly different approach to handling floating-point values, but if that turns out to be a clean implementation we may want to think about whether there are improvements from modelling integers similarly (only using `APInt` instead of `APFloat`).



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


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:63
+          Options.get("IgnoreAllFloatingPointValues", false)) {
+  // process set of ignored integer values
+  const std::vector<std::string> IgnoredIntegerValuesInput =
----------------
Comments should be grammatically correct, including capitalization and punctuation (elsewhere in this function as well).


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


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


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:85-88
+  llvm::SmallVector<float, SensibleNumberOfMagicValueExceptions>
+      IgnoredFloatingPointValues;
+  llvm::SmallVector<double, SensibleNumberOfMagicValueExceptions>
+      IgnoredDoublePointValues;
----------------
The model I was thinking of would have this be: `llvm::SmallVector<APFloat, SensibleNumberOfMagicValueExceptions> IgnoredFloatingPointValues;` so that you don't need to distinguish between floats and doubles (which also nicely supports long doubles, __float128, and _Float16).


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:63-67
+be configured using the :option:`IgnoredFloatingPointValues` option.  For each
+value in that set, bot the single-precision form and double-precision
+form are accepted (for example, if 3.14 is in the set, neither 3.14f nor 3.14
+will produce a warning). Scientific notation is supported for both source code
+input and option.
----------------
This isn't far off the mark, but I'd go with something like:
```
For each value in that set, the given string value is converted to a floating-point value representation used by the target architecture <NNN>. If a floating-point literal value compares equal to one of the converted values, then that literal is not diagnosed by this check. Because floating-point equality is used to determine whether to diagnose or not, the user needs to be aware of the details of floating-point representations for any values that cannot be precisely represented for their target architecture.
```
The <NNN> bit is where we can stick details we haven't worked out yet, like what rounding mode is used to perform the conversion and whether lossy conversions are allowed or ignored.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114