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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
