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