0x8000-0000 updated this revision to Diff 157871. 0x8000-0000 added a comment.
Add option to ignore all floating point values. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 Files: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp 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/cppcoreguidelines-avoid-magic-numbers.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,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 <typename T, int V> +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<int, 4> 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<int, INT_MACRO> 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 <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}}; + +// 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; +} Index: docs/clang-tidy/checks/readability-magic-numbers.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/readability-magic-numbers.rst @@ -0,0 +1,78 @@ +.. 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: + + * `Rule ES.45: Avoid “magic constants”; use symbolic constants in C++ Core Guidelines <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-magic>`_ + * 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` and `1` (and `-1`) integer values are accepted without a +warning. This can be overridden with the :option:`IgnoredIntegerValues` +option. Negative values are accepted if their absolute value is present in the +:option:`IgnoredIntegerValues` list. + +In addition, by default the `0.0` floating point value is accepted. There is no +configuration for accepting selected floating point values, primarily because most +floating point comparisons are not exact, and some of the exact ones are not +portable. The check for the floating point numbers can be disabled globally +by enabling :option:`IgnoreAllFloatingPointValues`. + +Options +------- + +.. option:: IgnoredIntegerValues + + Semicolon-separated list of magic positive integers that will be accepted + without a warning. + +.. option:: IgnoreAllFloatingPointValues + + Accept all floating point values without warning. + 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) <cert-oop11-cpp> cppcoreguidelines-avoid-goto + cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers> cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature> 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/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 <readability-magic-numbers.html>`_ +for more information. 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,85 @@ +//===--- 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 <llvm/ADT/SmallVector.h> +#include <vector> + +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 <typename L> + void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result, + const char *BoundName) { + const L *MatchedLiteral = Result.Nodes.getNodeAs<L>(BoundName); + if (!MatchedLiteral) + 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<std::string> IgnoredValuesInput; + bool IgnoreAllFloatingPointValues; + llvm::SmallVector<int64_t, 16> 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,124 @@ +//===--- 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. +// +//===----------------------------------------------------------------------===// +// +// A checker for magic numbers: integer or floating point literals embedded +// in the code, outside the definition of a constant or an enumeration. +// +//===----------------------------------------------------------------------===// + +#include "MagicNumbersCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/STLExtras.h" +#include <algorithm> + +using namespace clang::ast_matchers; +using namespace clang::ast_type_traits; + +namespace { + +bool isUsedToInitializeAConstant(const MatchFinder::MatchResult &Result, + const DynTypedNode &Node) { + + const auto *AsDecl = Node.get<clang::DeclaratorDecl>(); + if (AsDecl) { + if (AsDecl->getType().isConstQualified()) + return true; + + if (AsDecl->isImplicit()) + return true; + + return false; + } + + if (Node.get<clang::EnumConstantDecl>() != nullptr) + return true; + + return llvm::any_of(Result.Context->getParents(Node), + [&Result](const DynTypedNode &Parent) { + return isUsedToInitializeAConstant(Result, Parent); + }); +} + +} // namespace + +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))), + IgnoreAllFloatingPointValues( + Options.get("IgnoreAllFloatingPointValues", false)) { + 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); + if (!IgnoreAllFloatingPointValues) + Finder->addMatcher(floatLiteral().bind("float"), this); +} + +void MagicNumbersCheck::check(const MatchFinder::MatchResult &Result) { + checkBoundMatch<IntegerLiteral>(Result, "integer"); + checkBoundMatch<FloatingLiteral>(Result, "float"); +} + +bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result, + const Expr &ExprResult) const { + return llvm::any_of( + Result.Context->getParents(ExprResult), + [&Result](const 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<SubstNonTypeTemplateParmExpr>(); + }); +} + +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 SourceManager *SourceManager, + const IntegerLiteral *Literal) const { + const std::pair<FileID, unsigned> 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/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/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" @@ -39,6 +40,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck<AvoidGotoCheck>( "cppcoreguidelines-avoid-goto"); + CheckFactories.registerCheck<readability::MagicNumbersCheck>( + "cppcoreguidelines-avoid-magic-numbers"); CheckFactories.registerCheck<InterfacesGlobalInitCheck>( "cppcoreguidelines-interfaces-global-init"); CheckFactories.registerCheck<NarrowingConversionsCheck>(
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits