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,83 @@ +//===--- 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; + + bool isSyntheticValue(const clang::SourceManager *, + const FloatingLiteral *) const { + return false; + }; + bool isSyntheticValue(const clang::SourceManager *SourceManager, + const IntegerLiteral *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; + + if (isSyntheticValue(Result.SourceManager, MatchedLiteral)) + return; + + const StringRef LiteralSourceText = Lexer::getSourceText( + CharSourceRange::getTokenRange(MatchedLiteral->getSourceRange()), + *Result.SourceManager, getLangOpts()); + + diag(MatchedLiteral->getLocation(), + "%0 is a magic number; consider replacing it with a named constant") + << LiteralSourceText; + } + + 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,120 @@ +//===--- 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 isUsedToInitializeAConstant( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::ast_type_traits::DynTypedNode &Node) { + + const auto *AsDecl = Node.get(); + if (AsDecl) { + if (AsDecl->getType().isConstQualified()) + return true; + + if (AsDecl->isImplicit()) + return true; + + return false; + } + + if (Node.get() != nullptr) + return true; + + return llvm::any_of( + Result.Context->getParents(Node), + [&Result](const clang::ast_type_traits::DynTypedNode &Parent) { + return isUsedToInitializeAConstant(Result, Parent); + }); +} + +} // namespace + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +const char DefaultIgnoredIntegerValues[] = "0;1;"; + +MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoredValuesInput(utils::options::parseStringList( + Options.get("IgnoredIntegerValues", DefaultIgnoredIntegerValues))) { + IgnoredIntegerValues.resize(IgnoredValuesInput.size()); + llvm::transform(IgnoredValuesInput, IgnoredIntegerValues.begin(), + [](const std::string &Value) { return std::stoll(Value); }); + 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) || + // 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(); +} + +bool MagicNumbersCheck::isSyntheticValue( + const clang::SourceManager *SourceManager, + const IntegerLiteral *Literal) const { + const std::pair FileOffset = + SourceManager->getDecomposedLoc(Literal->getLocation()); + if (FileOffset.first.isInvalid()) { + return false; + } + + const StringRef BufferIdentifier = + SourceManager->getBuffer(FileOffset.first)->getBufferIdentifier(); + + return BufferIdentifier.empty(); +} + +} // 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 @@ -79,6 +79,7 @@ cert-msc51-cpp cert-oop11-cpp (redirects to performance-move-constructor-init) cppcoreguidelines-avoid-goto + cppcoreguidelines-avoid-magic-numbers cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) cppcoreguidelines-interfaces-global-init cppcoreguidelines-narrowing-conversions @@ -218,6 +219,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,68 @@ +.. 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'; + } + +By default only 0, 1 and -1 integer values are accepted without a warning. +This can be overriden with the 'IgnoredIntegerValues' option. In addition, +the 0.0 floating point value is accepted. There is no configuration for +accepted floating point values (primarily because floating point comparisons +are not exact). + +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,192 @@ +// RUN: %check_clang_tidy %s readability-magic-numbers %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}]}' \ +// RUN: -- + +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.1415926535f 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; + +int InitializedByMacro = INT_MACRO; + +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 ignored values (specified in the configuration above) + */ +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; + +const float Angles[] = {45.0f, 90.0f, 135.0f}; + +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}}; + +// Simulate the macro magic in Google Test internal headers +class AssertionHelper { +public: + AssertionHelper(const char *Message, int LineNumber) : Message(Message), LineNumber(LineNumber) {} + +private: + const char *Message; + int LineNumber; +}; + +#define ASSERTION_HELPER_AT(M, L) AssertionHelper(M, L) + +#define ASSERTION_HELPER(M) ASSERTION_HELPER_AT(M, __LINE__) + +void FunctionWithCompilerDefinedSymbol(void) { + ASSERTION_HELPER("here and now"); +} + +// prove that integer literals introduced by the compiler are accepted silently +extern int ConsumeString(const char *Input); + +const char *SomeStrings[] = {"alpha", "beta", "gamma"}; + +int TestCheckerOverreach() { + int Total = 0; + + for (const auto *Str : SomeStrings) + { + Total += ConsumeString(Str); + } + + return Total; +}