AlexanderLanin updated this revision to Diff 88447.
AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.
Applied review comments and added test cases regarding parenthesis, floats,
doubles, wide chars etc
https://reviews.llvm.org/D29692
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp
clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
test/clang-tidy/modernize-use-const-instead-of-define.cpp
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,67 @@
+// 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 BAD8 L'c'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD9 U'c'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD10 1U
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD11 1.5
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD12 1.4f
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD13 1.3L
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD14 ((-3))
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD15 -(-3)
+// 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 GOOD17 L"str"
+#define GOOD18 U"str"
+#define GOOD19 ()
+#define GOOD20 ++
+#define GOOD21 ++i
+#define GOOD22 k--
+#define GOOD23 m
+#define GOOD24 5-
+#define GOOD25 ((3)
+#define GOOD26 (3))
+#define GOOD27 )2(
+
+#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)
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,57 @@
+.. 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;
+ }
+
+Strings
+---------------
+
+Strings are excluded from this rule as they might be used for string
+concatenations. Example:
+
+.. code-block:: c++
+
+ #define A "A"
+ #define B "B"
+ char AB[3] = A B;
+
+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
+- MISRA C++ 16-2-2
+- JSF AV Rule 30
+- HIC++ 16.1.1
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/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
Improvements to clang-tidy
--------------------------
+- New `modernize-use-const-instead-of-define
+ <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-const-instead-of-define.html>`_ check
+
+ Finds uses of ``#define`` where a const value would be more appropriate.
+
- New `readability-misleading-indentation
<http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
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,97 @@
+//===--- 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;
+
+private:
+ Preprocessor *PP;
+ UseConstInsteadOfDefineCheck *Check;
+};
+
+} // namespace
+
+/// 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);
+}
+
+void UseConstInsteadOfDefineCheckPPCallbacks::MacroDefined(
+ const Token &MacroNameTok, const MacroDirective *MD) {
+
+ const MacroInfo *MI = MD->getMacroInfo();
+
+ bool IsConstantValue = false;
+ bool HasPrefix = false;
+ int ParanthesisLevel = 0;
+
+ for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
+ const Token &Tok = *TI;
+
+ // Numeric values may have `+` or `-` signs in front of them
+ // others like `~` are not so obvious to convert and depend on usage.
+ // Although there might be cases where the sign is actually used, those
+ // should be rare enough. e.g.
+ // `#define A +5`
+ // `int a = 5 A`;
+ if (!IsConstantValue && Tok.isOneOf(tok::plus, tok::minus)) {
+ HasPrefix = true;
+ } else if (Tok.is(tok::l_paren)) {
+ ++ParanthesisLevel;
+ } else if (Tok.is(tok::r_paren) && (ParanthesisLevel > 0)) {
+ --ParanthesisLevel;
+ }
+ // Chars with a `+` or `-` sign are rather rare and may have some special
+ // significance as shown above.
+ else if (Tok.is(tok::numeric_constant) ||
+ (!HasPrefix && isAnyCharLiteral(Tok))) {
+ IsConstantValue = true;
+ } else {
+ // Invalidate location as this `#define` is not a simple constant
+ // expression.
+ IsConstantValue = false;
+ break;
+ }
+ }
+
+ if (IsConstantValue && (ParanthesisLevel == 0)) {
+ Check->diag(MI->getDefinitionLoc(),
+ "use const variables instead of #define statements for "
+ "constant values");
+ }
+}
+
+void UseConstInsteadOfDefineCheck::registerPPCallbacks(
+ CompilerInstance &Compiler) {
+ Compiler.getPreprocessor().addPPCallbacks(
+ llvm::make_unique<UseConstInsteadOfDefineCheckPPCallbacks>(
+ &Compiler.getPreprocessor(), this));
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
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<UseAutoCheck>("modernize-use-auto");
CheckFactories.registerCheck<UseBoolLiteralsCheck>(
"modernize-use-bool-literals");
+ CheckFactories.registerCheck<UseConstInsteadOfDefineCheck>(
+ "modernize-use-const-instead-of-define");
CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
"modernize-use-default-member-init");
CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
