[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:23
+
+const char DefaultIgnoredValues[] = "0;1;2;10;100;";
+
----------------
Why 2, 10, and 100?


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:30-32
+  for (const std::string &IgnoredValue : IngnoredValuesInput) {
+    IngnoredValues.push_back(std::stoll(IgnoredValue));
+  }
----------------
This can be replaced with `llvm::transform(IgnoredValuesInput, IgnoredValues.begin(), std::stoll);`


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:33
+  }
+  std::sort(IngnoredValues.begin(), IngnoredValues.end());
+}
----------------
Please use `llvm::sort()` instead to help find non-determinism bugs. (I'm a bit surprised we don't have a range-based sort though.)


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:37
+void MagicNumbersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoredValues", DefaultIgnoredValues);
+}
----------------
`IgnoredIntegerValues` as the public facing option as well, unless you want to allow users to specify ignored floating-point values too (which could be useful as well).


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:41-42
+void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("ii"), this);
+  Finder->addMatcher(floatLiteral().bind("ff"), this);
+}
----------------
The names `ii` and `ff` could be a bit more user-friendly. Also, this can be written using a single matcher, I think.
`anyOf(integerLiteral().bind("integer"), floatLiteral().bind("float"))`


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:54
+      Result.Nodes.getNodeAs<IntegerLiteral>("ii");
+  if (MatchedInteger) {
+
----------------
Rather than indent everything for the typical case, I'd prefer this to be an early return.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:72
+
+    diag(MatchedInteger->getLocation(), "magic number integer literal %0")
+        << Str.data();
----------------
This diagnostic text doesn't really tell the user what's wrong with their code or how to fix it. Also, this function is largely duplicated in `checkFloatMatch()`. I think they should be combined where possible and the diagnostic should read: `'%0' is a magic number; consider replacing with a named constant` or something along those lines.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:118
+    const ast_type_traits::DynTypedNode &Node) const {
+  const VarDecl *AsVarDecl = Node.get<VarDecl>();
+  if (AsVarDecl) {
----------------
Can use `const auto *` here as the type is spelled out in the initialization.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:119-123
+  if (AsVarDecl) {
+    if (AsVarDecl->getType().isConstQualified()) {
+      return true;
+    }
+  }
----------------
The `if` statements can be combined into a single statement and the braces can be elided.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:124-126
+  const FieldDecl *AsFieldDecl = Node.get<FieldDecl>();
+  if (AsFieldDecl) {
+    if (AsFieldDecl->getType().isConstQualified()) {
----------------
Same here for `auto` and single statement.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:135-140
+  const SubstNonTypeTemplateParmExpr *AsTemplateArgument =
+      Node.get<SubstNonTypeTemplateParmExpr>();
+  if (AsTemplateArgument) {
+    return true;
+  }
+  return false;
----------------
`return Node.get<SubstNonTypeTemplateParmExpr> != nullptr;` However, that makes me wonder why this should be a function at all.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:147
+
+  if (isConstantDefinition(Node)) {
+    return true;
----------------
Elide braces


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:151-158
+  for (const ast_type_traits::DynTypedNode &Parent :
+       Result.Context->getParents(Node)) {
+    if (isEventuallyConstant(Result, Parent)) {
+      return true;
+    }
+  }
+
----------------
I think this could be replaced by a call to `llvm::any_of()`


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:164
+    const ast_type_traits::DynTypedNode &Node) const {
+  const EnumConstantDecl *AsEnumConstantDecl = Node.get<EnumConstantDecl>();
+  if (AsEnumConstantDecl)
----------------
No need for this local variable.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:168-174
+  for (const ast_type_traits::DynTypedNode &Parent :
+       Result.Context->getParents(Node)) {
+    if (isEnumerationDefinition(Result, Parent))
+      return true;
+  }
+
+  return false;
----------------
`llvm::any_of()`


================
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:183-186
+    /*
+     * Ignore this instance, because this is the report where the template is
+     * defined, not where it is instantiated.
+     */
----------------
No need for fancy C-style comments here; can just use `//`.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:15
+
+#include <unordered_set>
+
----------------
This include can be removed.


================
Comment at: clang-tidy/readability/MagicNumbersCheck.h:53
+  const std::vector<std::string> IngnoredValuesInput;
+  std::vector<int64_t> IngnoredValues;
+};
----------------
I think this should be `IgnoredIntegerValue` given that it only is checked for integers. That should be reflected in the documentation as well.


================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:69-70
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<MagicNumbersCheck>(
+        "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
----------------
I think this should also be registered as a C++ Core Guideline checker, as I believe it covers at least the integer and floating-point parts of https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-magic


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:1
+.. title:: clang-tidy - readability-magic-numbers
+
----------------
This doesn't document the user-facing configuration options, but it should.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:4
+readability-magic-numbers
+==========================
+
----------------
Underlining is too long.


================
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:6
+
+Detects magic numbers, integer or floating point literal that are embedded in
+code and not introduced via constants or symbols.
----------------
literal -> literals


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114