Index: clang-tidy/modernize/CMakeLists.txt =================================================================== --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeLists.txt @@ -16,6 +16,7 @@ ShrinkToFitCheck.cpp UseAutoCheck.cpp UseBoolLiteralsCheck.cpp + UseConstInsteadOfDefineCheck.cpp UseDefaultMemberInitCheck.cpp UseEmplaceCheck.cpp UseEqualsDefaultCheck.cpp Index: clang-tidy/modernize/ModernizeTidyModule.cpp =================================================================== --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -22,6 +22,7 @@ #include "ShrinkToFitCheck.h" #include "UseAutoCheck.h" #include "UseBoolLiteralsCheck.h" +#include "UseConstInsteadOfDefineCheck.h" #include "UseDefaultMemberInitCheck.h" #include "UseEmplaceCheck.h" #include "UseEqualsDefaultCheck.h" @@ -57,6 +58,8 @@ CheckFactories.registerCheck("modernize-use-auto"); CheckFactories.registerCheck( "modernize-use-bool-literals"); + CheckFactories.registerCheck( + "modernize-use-const-instead-of-define"); CheckFactories.registerCheck( "modernize-use-default-member-init"); CheckFactories.registerCheck("modernize-use-emplace"); Index: clang-tidy/modernize/UseConstInsteadOfDefineCheck.h =================================================================== --- /dev/null +++ clang-tidy/modernize/UseConstInsteadOfDefineCheck.h @@ -0,0 +1,40 @@ +//===--- UseConstInsteadOfDefineCheck.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_MODERNIZE_USE_CONST_INSTEAD_OF_DEFINE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_CONST_INSTEAD_OF_DEFINE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// C++ const variables should be preferred over #define statements +/// +/// const variables obey type checking and scopes +/// +/// Further documentation e.g. at +/// https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions +/// http://www.stroustrup.com/JSF-AV-rules.pdf +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-const-instead-of-define.html +class UseConstInsteadOfDefineCheck : public ClangTidyCheck { +public: + UseConstInsteadOfDefineCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(CompilerInstance &Compiler) override; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_CONST_INSTEAD_OF_DEFINE_H Index: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp @@ -0,0 +1,119 @@ +//===--- UseConstInsteadOfDefineCheck.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 "UseConstInsteadOfDefineCheck.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" + +namespace clang { +namespace tidy { +namespace modernize { + +namespace { +class UseConstInsteadOfDefineCheckPPCallbacks : public PPCallbacks { +public: + UseConstInsteadOfDefineCheckPPCallbacks(Preprocessor *PP, + UseConstInsteadOfDefineCheck *Check) + : PP(PP), Check(Check) {} + + void MacroDefined(const Token &MacroNameTok, + const MacroDirective *MD) override { + checkForConstantValues(MacroNameTok, MD->getMacroInfo()); + } + +private: + /// performs the actual check + void checkForConstantValues(const Token &MacroNameTok, const MacroInfo *MI); + + Preprocessor *PP; + UseConstInsteadOfDefineCheck *Check; +}; + +} // namespace + +/// numeric values may have + or - signs in front of them +/// others like ~ are not so obvious and depend on usage +static bool isReasonableNumberPrefix(const Token &T) { + return T.isOneOf(tok::plus, tok::minus); +} + +/// Is T some sort of constant numeric value? +static bool isAnyNumericLiteral(const Token &T) { return T.is(tok::numeric_constant); } + +/// Is T some sort of constant char? +static bool isAnyCharLiteral(const Token &T) { + return T.isOneOf(tok::char_constant, tok::wide_char_constant, + tok::utf8_char_constant, tok::utf16_char_constant, + tok::utf32_char_constant); +} + +/// Is T some sort of constant value? +static bool isConstantValue(const Token &T) { + return isAnyNumericLiteral(T) || isAnyCharLiteral(T); +} + +/// values may be in (parenthesis) +static bool isAnyParenthesis(const Token &T) { + return T.isOneOf(tok::l_paren, tok::r_paren); +} + +void UseConstInsteadOfDefineCheckPPCallbacks::checkForConstantValues( + const Token &MacroNameTok, const MacroInfo *MI) { + SourceLocation Loc; + + bool hasPrefix = false; + bool hasValue = false; + + for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) { + const Token &Tok = *TI; + + if (!hasPrefix && isReasonableNumberPrefix(Tok)) { + hasPrefix = true; + + // start at number prefix like the + in +7 + Loc = Tok.getLocation(); + } + // numbers may have a prefix, however chars with a prefix are rather + // strange... let's not touch them + else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) || + (!hasPrefix && isAnyCharLiteral(Tok))) { + // prefix shall not be accepted anymore after any number + hasPrefix = true; + hasValue = true; + + // Store location in case this is the first appearance + if (Loc.isInvalid()) { + Loc = Tok.getLocation(); + } + } else { + // invalidate location, this #define is not a simple constant expression + Loc = SourceLocation(); + break; + } + } + + // loc is valid with a prefix only, so also check for an actual value/number + if (Loc.isValid() && hasValue) { + Check->diag(Loc, "use const variables instead of #define statements for " + "constant values"); + } +} + + +void UseConstInsteadOfDefineCheck::registerPPCallbacks( + CompilerInstance &Compiler) { + Compiler.getPreprocessor().addPPCallbacks( + llvm::make_unique( + &Compiler.getPreprocessor(), this)); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,12 @@ Improvements to clang-tidy -------------------------- + +- New `modernize-use-const-instead-of-define + `_ check + + Finds uses of ``#define`` where a const value would be more appropriate. + - New `safety-no-assembler `_ check Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -112,6 +112,7 @@ modernize-shrink-to-fit modernize-use-auto modernize-use-bool-literals + modernize-use-const-instead-of-define modernize-use-default-member-init modernize-use-emplace modernize-use-equals-default Index: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst @@ -0,0 +1,41 @@ +.. title:: clang-tidy - modernize-use-const-instead-of-define + +modernize-use-const-instead-of-define +===================================== + +C++ const variables should be preferred over ``#define`` directives as +``#define`` does not obey type checking and scope rules. + +Example +------- + +.. code-block:: c++ + + `// calc.h + namespace RealCalculation { + #define PI 3.14159 + } + + // quick.h + namespace QuickCalculation { + #define PI 1 + } + + // calc.cpp + #include "calc.h" + #include "quick.h" + + double calc(const double r) { + return 2*PI*r*r; + } + +Code guidelines +--------------- + +While many C++ code guidelines like to prohibit macros completely with the +exception of include guards, that's a rather strict limitation. +Disallowing simple numbers should not require any significant code change and +may even offer fix-it suggestions in the future. + +See also: +https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions Index: test/clang-tidy/modernize-use-const-instead-of-define.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-const-instead-of-define.cpp @@ -0,0 +1,40 @@ +// RUN: %check_clang_tidy %s modernize-use-const-instead-of-define %t + +// Although there might be cases where the - sign is actually used those +// should be rare enough. e.g. int a = 5 BAD1; +#define BAD1 -1 +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD2 2 +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD3(A) 0 +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD4(X) (7) +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD5(X) +4 +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD6 'x' +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] +#define BAD7 0xff +// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define] + +#define GOOD1 - +#define GOOD2 (1+2) +#define GOOD3(A) #A +#define GOOD4(A,B) A+B +#define GOOD5(T) ((T*)0) +#define GOOD6(type) (type(123)) +#define GOOD7(car, ...) car +#define GOOD8 a[5] +#define GOOD9(x) ;x; +#define GOOD10 ;-2; +#define GOOD11 =2 +#define GOOD12 +'a' +#define GOOD13 -'z' +#define GOOD14 !3 +#define GOOD15 ~-1 +#define GOOD16 "str" + +#define NOT_DETECTED_YET_1(x) ((unsigned char)(0xff)) +#define NOT_DETECTED_YET_2 (int)7 +#define NOT_DETECTED_YET_3 int(-5) +#define NOT_DETECTED_YET_4 (int)(0)