Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp =================================================================== --- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "../misc/UnconventionalAssignOperatorCheck.h" +#include "../readability/MagicNumbersCheck.h" #include "AvoidGotoCheck.h" #include "InterfacesGlobalInitCheck.h" #include "NarrowingConversionsCheck.h" @@ -41,6 +42,8 @@ "cppcoreguidelines-avoid-goto"); CheckFactories.registerCheck( "cppcoreguidelines-interfaces-global-init"); + CheckFactories.registerCheck( + "cppcoreguidelines-avoid-magic-numbers"); CheckFactories.registerCheck( "cppcoreguidelines-narrowing-conversions"); CheckFactories.registerCheck("cppcoreguidelines-no-malloc"); 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 Index: clang-tidy/readability/MagicNumbersCheck.h =================================================================== --- /dev/null +++ clang-tidy/readability/MagicNumbersCheck.h @@ -0,0 +1,74 @@ +//===--- 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 +#include + +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: + bool isConstant(const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::Expr &ExprResult) const; + + bool isIgnoredValue(const IntegerLiteral *Literal) const; + bool isIgnoredValue(const FloatingLiteral *Literal) const; + + template + void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result, + const char *boundName) { + const L *MatchedLiteral = Result.Nodes.getNodeAs(boundName); + if (MatchedLiteral == nullptr) + return; + + if (Result.SourceManager->isMacroBodyExpansion( + MatchedLiteral->getLocation())) + return; + + if (isIgnoredValue(MatchedLiteral)) + return; + + if (isConstant(Result, *MatchedLiteral)) + return; + + auto Value = MatchedLiteral->getValue(); + SmallVector ValueAsString(32, '\0'); + ValueAsString.resize(0); + Value.toString(ValueAsString, 10, true); + + diag(MatchedLiteral->getLocation(), + "%0 is a magic number; consider replacing it with a named constant") + << ValueAsString.data(); + } + + std::vector IgnoredValuesInput; + llvm::SmallVector IgnoredIntegerValues; +}; + +} // 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,121 @@ +//===--- 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 "llvm/ADT/STLExtras.h" +#include + +namespace { + +bool isConstantDefinition(const clang::ast_type_traits::DynTypedNode &Node) { + const auto *AsVarDecl = Node.get(); + if (AsVarDecl && AsVarDecl->getType().isConstQualified()) + return true; + + const auto *AsFieldDecl = Node.get(); + if (AsFieldDecl && AsFieldDecl->getType().isConstQualified()) + return true; + + return false; +} + +bool isUsedToInitializeAConstant( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::ast_type_traits::DynTypedNode &Node) { + + if (isConstantDefinition(Node)) + return true; + + return llvm::any_of( + Result.Context->getParents(Node), + [&Result](const clang::ast_type_traits::DynTypedNode &Parent) { + return isUsedToInitializeAConstant(Result, Parent); + }); +} + +bool isEnumerationDefinition( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::ast_type_traits::DynTypedNode &Node) { + if (Node.get() != nullptr) + return true; + + return llvm::any_of( + Result.Context->getParents(Node), + [&Result](const clang::ast_type_traits::DynTypedNode &Parent) { + return isEnumerationDefinition(Result, Parent); + }); +} + +} // namespace + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +const char DefaultIgnoredIntegerValues[] = "0;1;2;10;100;"; + +MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoredValuesInput(utils::options::parseStringList( + Options.get("IgnoredIntegerValues", DefaultIgnoredIntegerValues))) { + IgnoredIntegerValues.reserve(IgnoredValuesInput.size()); + for (const auto &IgnoredValue : IgnoredValuesInput) { + IgnoredIntegerValues.push_back(std::stoll(IgnoredValue)); + } + llvm::sort(IgnoredIntegerValues.begin(), IgnoredIntegerValues.end()); +} + +void MagicNumbersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoredIntegerValues", DefaultIgnoredIntegerValues); +} + +void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(integerLiteral().bind("integer"), this); + Finder->addMatcher(floatLiteral().bind("float"), this); +} + +void MagicNumbersCheck::check(const MatchFinder::MatchResult &Result) { + checkBoundMatch(Result, "integer"); + checkBoundMatch(Result, "float"); +} + +bool MagicNumbersCheck::isConstant( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::Expr &ExprResult) const { + return llvm::any_of( + Result.Context->getParents(ExprResult), + [&Result](const clang::ast_type_traits::DynTypedNode &Parent) { + return isUsedToInitializeAConstant(Result, Parent) || + isEnumerationDefinition(Result, Parent) || + // Ignore this instance, because this match reports the location + // where the template is defined, not where it is instantiated. + (Parent.get() != nullptr); + }); +} + +bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const { + llvm::APInt IntValue = Literal->getValue(); + int64_t Value = IntValue.getZExtValue(); + return std::binary_search(IgnoredIntegerValues.begin(), + IgnoredIntegerValues.end(), Value); +} + +bool MagicNumbersCheck::isIgnoredValue(const FloatingLiteral *Literal) const { + llvm::APFloat FloatValue = Literal->getValue(); + return FloatValue.isZero(); +} + +} // namespace readability +} // namespace tidy +} // namespace clang 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( "readability-inconsistent-declaration-parameter-name"); + CheckFactories.registerCheck( + "readability-magic-numbers"); CheckFactories.registerCheck( "readability-misleading-indentation"); CheckFactories.registerCheck( 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 + ` 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 ` check. Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst @@ -0,0 +1,10 @@ +.. title:: clang-tidy - cppcoreguidelines-avoid-magic-numbers +.. meta:: + :http-equiv=refresh: 5;URL=readability-magic-numbers.html + +cppcoreguidelines-avoid-magic-numbers +===================================== + +The cppcoreguidelines-avoid-magic-numbers check is an alias, please see +`readability-magic-numbers `_ +for more information. 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/clang-tidy/checks/readability-magic-numbers.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/readability-magic-numbers.rst @@ -0,0 +1,62 @@ +.. title:: clang-tidy - readability-magic-numbers + +readability-magic-numbers +========================= + +Detects magic numbers, integer or floating point literals 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() { + return -3; // FILENOTFOUND + } + + for (int mm = 1; mm <= 12; ++mm) { + std::cout << month[mm] << '\n'; + } + +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() { + return E_FILE_NOT_FOUND; + } + + for (int mm = 1; mm <= MONTHS_IN_A_YEAR; ++mm) { + std::cout << month[mm] << '\n'; + } + + +Options +------- + +.. option:: IgnoredIntegerValues + + Semicolon-separated list of magic integers that will be accepted without a + warning. 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 +struct ValueBucket { + T value[V]; +}; + +int BadGlobalInt = 5; +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + +int IntSquarer(int param) { + return param * param; +} + +void BuggyFunction() { + int BadLocalInt = 6; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + + (void)IntSquarer(7); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + + int LocalArray[8]; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + + for (int ii = 0; ii < 8; ++ii) + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + { + LocalArray[ii] = 3 * ii; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + } + + ValueBucket Bucket; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers] +} + +class TwoIntContainer { +public: + TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {} + // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + + int getValue() const; + +private: + int oneMember = 9; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + + int anotherMember; + + int yetAnotherMember; + + const int oneConstant = 2; + + const int anotherConstant; +}; + +int ValueArray[] = {3, 5}; +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers] +// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + +float FloatPiVariable = 3.1415926535f; +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.141592741 is a magic number; consider replacing it with a named constant [readability-magic-numbers] +double DoublePiVariable = 6.283185307; +// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers] + +int getAnswer() { + if (ValueArray[0] < ValueArray[1]) + return ValueArray[1]; + + return -3; // FILENOTFOUND + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers] +} + +/* + * Clean code + */ + +#define INT_MACRO 5 + +const int GoodGlobalIntConstant = 42; + +constexpr int AlsoGoodGlobalIntConstant = 42; + +void SolidFunction() { + const int GoodLocalIntConstant = 43; + + (void)IntSquarer(GoodLocalIntConstant); + + int LocalArray[INT_MACRO]; + + ValueBucket 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 +struct Point { + T x; + T y; + + explicit Point(T xval, T yval) noexcept : x{xval}, y{yval} { + } +}; + +template +struct Dimension { + T x; + T y; + + explicit Dimension(T xval, T yval) noexcept : x{xval}, y{yval} { + } +}; + +template +struct Rectangle { + Point origin; + Dimension size; + T rotation; // angle of rotation around origin + + Rectangle(Point origin_, Dimension size_, T rotation_ = 0) noexcept : origin{origin_}, size{size_}, rotation{rotation_} { + } + + bool contains(Point point) const; +}; + +} // namespace geometry + +const geometry::Rectangle mandelbrotCanvas{geometry::Point{-2.5, -1}, geometry::Dimension{3.5, 2}};