0x8000-0000 updated this revision to Diff 155994.
0x8000-0000 added a comment.
Address several review comments. Create alias for
cppcoreguidelines-avoid-magic-numbers .
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MagicNumbersCheck.cpp
clang-tidy/readability/MagicNumbersCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-magic-numbers.rst
test/clang-tidy/readability-magic-numbers.cpp
Index: test/clang-tidy/readability-magic-numbers.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,151 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t
+
+template <typename T, int V>
+struct ValueBucket {
+ T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 5 [readability-magic-numbers]
+
+int IntSquarer(int param) {
+ return param * param;
+}
+
+void BuggyFunction() {
+ int BadLocalInt = 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 6 [readability-magic-numbers]
+
+ (void)IntSquarer(7);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 7 [readability-magic-numbers]
+
+ int LocalArray[8];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: magic number integer literal 8 [readability-magic-numbers]
+
+ for (int ii = 0; ii < 8; ++ii)
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: magic number integer literal 8 [readability-magic-numbers]
+ {
+ LocalArray[ii] = 3 * ii;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: magic number integer literal 3 [readability-magic-numbers]
+ }
+
+ ValueBucket<int, 4> Bucket;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: magic number integer literal 4 [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+ TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: magic number integer literal 6 [readability-magic-numbers]
+
+ int getValue() const;
+
+private:
+ int oneMember = 9;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: magic number integer literal 9 [readability-magic-numbers]
+
+ int anotherMember;
+
+ int yetAnotherMember;
+
+ const int oneConstant = 2;
+
+ const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 3 [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: magic number integer literal 5 [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: magic number float literal 3.141592741 [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: magic number float literal 6.283185307 [readability-magic-numbers]
+
+int getAnswer() {
+ if (ValueArray[0] < ValueArray[1])
+ return ValueArray[1];
+
+ return -3; // FILENOTFOUND
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: magic number integer literal 3 [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntContant = 42;
+
+constexpr int AlsoGoodGlobalIntContant = 42;
+
+void SolidFunction() {
+ const int GoodLocalIntContant = 43;
+
+ (void)IntSquarer(GoodLocalIntContant);
+
+ int LocalArray[INT_MACRO];
+
+ ValueBucket<int, INT_MACRO> Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for grandfathered values
+ */
+int GrandfatheredValues[] = {0, 1, 2, 10, 100, -1, -10, -100};
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+ STARTER,
+ ALPHA = 3,
+ BETA = 1 << 5,
+};
+
+const float FloatPiConstant = 3.1415926535f;
+const double DoublePiConstant = 6.283185307;
+
+double DoubleZeroIsAccepted = 0.0;
+float FloatZeroIsAccepted = 0.0f;
+
+namespace geometry {
+
+template <typename T>
+struct Point {
+ T x;
+ T y;
+
+ explicit Point(T xval, T yval) noexcept : x{xval}, y{yval} {
+ }
+};
+
+template <typename T>
+struct Dimension {
+ T x;
+ T y;
+
+ explicit Dimension(T xval, T yval) noexcept : x{xval}, y{yval} {
+ }
+};
+
+template <typename T>
+struct Rectangle {
+ Point<T> origin;
+ Dimension<T> size;
+ T rotation; // angle of rotation around origin
+
+ Rectangle(Point<T> origin_, Dimension<T> size_, T rotation_ = 0) noexcept : origin{origin_}, size{size_}, rotation{rotation_} {
+ }
+
+ bool contains(Point<T> point) const;
+};
+
+} // namespace geometry
+
+const geometry::Rectangle<double> mandelbrotCanvas{geometry::Point<double>{-2.5, -1}, geometry::Dimension<double>{3.5, 2}};
Index: docs/clang-tidy/checks/readability-magic-numbers.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/readability-magic-numbers.rst
@@ -0,0 +1,51 @@
+.. title:: clang-tidy - readability-magic-numbers
+
+readability-magic-numbers
+==========================
+
+Detects magic numbers, integer or floating point literal that are embedded in
+code and not introduced via constants or symbols.
+
+Many coding guidelines advise replacing the magic values with symbolic
+constants to improve readability. Here are a few references:
+
+ * Item 17 in "C++ Coding Standards: 101 Rules, Guidelines and Best
+ Practices" by Herb Sutter and Andrei Alexandrescu
+ * Chapter 17 in "Clean Code - A handbook of agile software craftsmanship."
+ by Robert C. Martin
+ * Rule 20701 in "TRAIN REAL TIME DATA PROTOCOL Coding Rules" by Armin-Hagen
+ Weiss, Bombardier
+ * http://wiki.c2.com/?MagicNumber
+
+
+Examples of magic values:
+
+.. code-block:: c++
+
+ double circleArea = 3.1415926535 * radius * radius;
+
+ double totalCharge = 1.08 * itemPrice;
+
+ int getAnswer() {
+ if (condition)
+ return someGoodValue;
+
+ return -3; // FILENOTFOUND
+ }
+
+Example with magic values refactored:
+
+.. code-block:: c++
+
+ double circleArea = M_PI * radius * radius;
+
+ const double TAX_RATE = 0.08; // or make it variable and read from a file
+
+ double totalCharge = (1.0 + TAX_RATE) * itemPrice;
+
+ int getAnswer() {
+ if (condition)
+ return someGoodValue;
+
+ return E_FILE_NOT_FOUND;
+ }
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -218,6 +218,7 @@
readability-identifier-naming
readability-implicit-bool-conversion
readability-inconsistent-declaration-parameter-name
+ readability-magic-numbers
readability-misleading-indentation
readability-misplaced-array-index
readability-named-parameter
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -170,6 +170,12 @@
Warns or suggests alternatives if SIMD intrinsics are used which can be replaced by
``std::experimental::simd`` operations.
+- New :doc:`readability-magic-numbers
+ <clang-tidy/checks/readability-magic-numbers>` check.
+
+ Detects usage of magic numbers, numbers that are used as literals instead of
+ introduced via constants or symbols.
+
- New :doc:`readability-simplify-subscript-expr
<clang-tidy/checks/readability-simplify-subscript-expr>` check.
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
#include "IdentifierNamingCheck.h"
#include "ImplicitBoolConversionCheck.h"
#include "InconsistentDeclarationParameterNameCheck.h"
+#include "MagicNumbersCheck.h"
#include "MisleadingIndentationCheck.h"
#include "MisplacedArrayIndexCheck.h"
#include "NamedParameterCheck.h"
@@ -65,6 +66,8 @@
"readability-implicit-bool-conversion");
CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
"readability-inconsistent-declaration-parameter-name");
+ CheckFactories.registerCheck<MagicNumbersCheck>(
+ "readability-magic-numbers");
CheckFactories.registerCheck<MisleadingIndentationCheck>(
"readability-misleading-indentation");
CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
Index: clang-tidy/readability/MagicNumbersCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/readability/MagicNumbersCheck.h
@@ -0,0 +1,60 @@
+//===--- MagicNumbersCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAGICNUMBERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAGICNUMBERSCHECK_H
+
+#include "../ClangTidy.h"
+
+#include <unordered_set>
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detects magic numbers, integer and floating point literals embedded in code.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-magic-numbers.html
+class MagicNumbersCheck : public ClangTidyCheck {
+public:
+ MagicNumbersCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ void checkIntegerMatch(const ast_matchers::MatchFinder::MatchResult &Result);
+ void checkFloatMatch(const ast_matchers::MatchFinder::MatchResult &Result);
+
+ bool isIgnoredIntegerValue(const llvm::APInt &IntValue) const;
+ bool isIgnoredFloatValue(const llvm::APFloat &FloatValue) const;
+
+ bool isConstantDefinition(const ast_type_traits::DynTypedNode &Node) const;
+ bool isTemplateArgumentAtSourceLocation(
+ const ast_type_traits::DynTypedNode &Node) const;
+ bool
+ isEventuallyConstant(const ast_matchers::MatchFinder::MatchResult &Result,
+ const ast_type_traits::DynTypedNode &Node) const;
+ bool
+ isEnumerationDefinition(const ast_matchers::MatchFinder::MatchResult &Result,
+ const ast_type_traits::DynTypedNode &Node) const;
+
+ bool isConstant(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &ExprResult) const;
+
+ const std::vector<std::string> IngnoredValuesInput;
+ std::vector<int64_t> IngnoredValues;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAGICNUMBERSCHECK_H
Index: clang-tidy/readability/MagicNumbersCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/readability/MagicNumbersCheck.cpp
@@ -0,0 +1,202 @@
+//===--- MagicNumbersCheck.cpp - clang-tidy-------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MagicNumbersCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+const char DefaultIgnoredValues[] = "0;1;2;10;100;";
+
+MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IngnoredValuesInput(utils::options::parseStringList(
+ Options.get("IgnoredValues", DefaultIgnoredValues))) {
+ IngnoredValues.reserve(IngnoredValuesInput.size());
+ for (const std::string &IgnoredValue : IngnoredValuesInput) {
+ IngnoredValues.push_back(std::stoll(IgnoredValue));
+ }
+ std::sort(IngnoredValues.begin(), IngnoredValues.end());
+}
+
+void MagicNumbersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IgnoredValues", DefaultIgnoredValues);
+}
+
+void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(integerLiteral().bind("ii"), this);
+ Finder->addMatcher(floatLiteral().bind("ff"), this);
+}
+
+void MagicNumbersCheck::check(const MatchFinder::MatchResult &Result) {
+ checkIntegerMatch(Result);
+ checkFloatMatch(Result);
+}
+
+void MagicNumbersCheck::checkIntegerMatch(
+ const MatchFinder::MatchResult &Result) {
+ const IntegerLiteral *MatchedInteger =
+ Result.Nodes.getNodeAs<IntegerLiteral>("ii");
+ if (MatchedInteger) {
+
+ if (Result.SourceManager->isMacroBodyExpansion(
+ MatchedInteger->getLocation()))
+ return;
+
+ llvm::APInt Value = MatchedInteger->getValue();
+ if (isIgnoredIntegerValue(Value))
+ return;
+
+ if (isConstant(Result, *MatchedInteger))
+ return;
+
+ SmallVector<char, 32> Str(32, '\0');
+ Str.resize(0);
+
+ Value.toString(Str, 10, true);
+
+ diag(MatchedInteger->getLocation(), "magic number integer literal %0")
+ << Str.data();
+ }
+}
+
+void MagicNumbersCheck::checkFloatMatch(
+ const MatchFinder::MatchResult &Result) {
+
+ const FloatingLiteral *MatchedFloat =
+ Result.Nodes.getNodeAs<FloatingLiteral>("ff");
+ if (MatchedFloat) {
+
+ if (Result.SourceManager->isMacroBodyExpansion(MatchedFloat->getLocation()))
+ return;
+
+ llvm::APFloat Value = MatchedFloat->getValue();
+ if (isIgnoredFloatValue(Value))
+ return;
+
+ if (isConstant(Result, *MatchedFloat))
+ return;
+
+ SmallVector<char, 32> Str(32, '\0');
+ Str.resize(0);
+
+ Value.toString(Str, 10, true);
+
+ diag(MatchedFloat->getLocation(), "magic number float literal %0")
+ << Str.data();
+ }
+}
+
+bool MagicNumbersCheck::isIgnoredIntegerValue(
+ const llvm::APInt &IntValue) const {
+ int64_t Value = IntValue.getZExtValue();
+ return std::binary_search(IngnoredValues.begin(), IngnoredValues.end(),
+ Value);
+}
+
+bool MagicNumbersCheck::isIgnoredFloatValue(
+ const llvm::APFloat &FloatValue) const {
+ return FloatValue.isZero();
+}
+
+bool MagicNumbersCheck::isConstantDefinition(
+ const ast_type_traits::DynTypedNode &Node) const {
+ const VarDecl *AsVarDecl = Node.get<VarDecl>();
+ if (AsVarDecl) {
+ if (AsVarDecl->getType().isConstQualified()) {
+ return true;
+ }
+ }
+ const FieldDecl *AsFieldDecl = Node.get<FieldDecl>();
+ if (AsFieldDecl) {
+ if (AsFieldDecl->getType().isConstQualified()) {
+ return true;
+ }
+ }
+ return false;
+}
+
+bool MagicNumbersCheck::isTemplateArgumentAtSourceLocation(
+ const ast_type_traits::DynTypedNode &Node) const {
+ const SubstNonTypeTemplateParmExpr *AsTemplateArgument =
+ Node.get<SubstNonTypeTemplateParmExpr>();
+ if (AsTemplateArgument) {
+ return true;
+ }
+ return false;
+}
+
+bool MagicNumbersCheck::isEventuallyConstant(
+ const MatchFinder::MatchResult &Result,
+ const ast_type_traits::DynTypedNode &Node) const {
+
+ if (isConstantDefinition(Node)) {
+ return true;
+ }
+
+ for (const ast_type_traits::DynTypedNode &Parent :
+ Result.Context->getParents(Node)) {
+ if (isEventuallyConstant(Result, Parent)) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+bool MagicNumbersCheck::isEnumerationDefinition(
+ const MatchFinder::MatchResult &Result,
+ const ast_type_traits::DynTypedNode &Node) const {
+ const EnumConstantDecl *AsEnumConstantDecl = Node.get<EnumConstantDecl>();
+ if (AsEnumConstantDecl)
+ return true;
+
+ for (const ast_type_traits::DynTypedNode &Parent :
+ Result.Context->getParents(Node)) {
+ if (isEnumerationDefinition(Result, Parent))
+ return true;
+ }
+
+ return false;
+}
+
+bool MagicNumbersCheck::isConstant(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &ExprResult) const {
+ for (const ast_type_traits::DynTypedNode &Parent :
+ Result.Context->getParents(ExprResult)) {
+
+ /*
+ * Ignore this instance, because this is the report where the template is
+ * defined, not where it is instantiated.
+ */
+ if (isTemplateArgumentAtSourceLocation(Parent))
+ return true;
+
+ if (isEventuallyConstant(Result, Parent))
+ return true;
+
+ if (isEnumerationDefinition(Result, Parent))
+ return true;
+ }
+
+ return false;
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tidy/readability/CMakeLists.txt
+++ clang-tidy/readability/CMakeLists.txt
@@ -11,6 +11,7 @@
IdentifierNamingCheck.cpp
ImplicitBoolConversionCheck.cpp
InconsistentDeclarationParameterNameCheck.cpp
+ MagicNumbersCheck.cpp
MisleadingIndentationCheck.cpp
MisplacedArrayIndexCheck.cpp
NamedParameterCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
