[clang-tidy] add check modernize-use-const-instead-of-define

[AlexanderLanin]

Summary

Suggestion for a new check that will warn on #defines that should rather be constant values. Const variables should be preferred as #define does not obey type checking and scope rules.

Please feel free to criticize as strict as you like, this is my very first patch to llvm/clang. Also I'm not sure about the check name itself...

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

This check is also should be included into cppcoreguidelines module. Or may be moved there?

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

42

In LLVM Functions should be static, not inside anonymous namespace. Same below.

68

Unnecessary empty line.

docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst

7

If I'm not mistaken, preprocessor has directives, not statements. Please also enclose #define in ``.

10

A should be capitalized. Please also use code-block.

test/clang-tidy/modernize-use-const-instead-of-define.cpp

20

Unnecessary empty line.

39

Unnecessary empty line.

[malcolm.parsons]

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

21

s/MacroParentheses/ConstantValues/ ?

docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst

12

s/voif/void/
Why is this in a function anyway?

test/clang-tidy/modernize-use-const-instead-of-define.cpp

4

s/allthough/Although/

7

Check the message?

[AlexanderLanin]

Not sure about CppCoreGuidelines as several guidelines have the same rule and I only used CppCoreGuidelines as it's convenient to link a specific rule. But I can move it if you like?!

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

42

mhh copied from MacroParentheses check

docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst

12

that's why I wrote strange example ;-)
Hope the new one is better ?!

test/clang-tidy/modernize-use-const-instead-of-define.cpp

7

I didn't check the exact message since I changed the wording several times and it's currently the same message for everything that's detected anyway. Shall I check the exact message anyway?

[AlexanderLanin]

Fixes as reported in review

[aaron.ballman]

One big concern I have with this is the potential to introduce ODR violations into the user's code. We may want to limit this check to only trigger on macros that are defined in the primary source file of the TU rather than something in the include stack.

One other problem with this advice is that a constant variable isn't always the same thing as a macro replacement value. For instance, a const variable requires the ability to take the address of the variable, which means that space must be reserved for it at runtime. A frequent way around this is to use enumerator rather than constant variables for integral types. The enumerations are not prone to ODR violations and enumerators are not things with storage.

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

41

Make sure the comments have proper capitalization and punctuation, here and elsewhere.

43

I don't think most of these helper functions really clarify the code all that much (except for perhaps isAnyCharLiteral(), but even that's debatable). I would just fold these into the code.

83–84

I don't really agree with this comment. L'1' is a reasonable character constant and not at all strange. You should add tests for that case and perhaps clarify the comment.

86

What about string literals? e.g., #define NAME "foobar"

clang-tidy/modernize/UseConstInsteadOfDefineCheck.h

19–21

Missing punctuation at the end of the sentences in these comments.

docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst

14

Extraneous ` before the comment.

[AlexanderLanin]

Thanks for the feedback. As I'm new to this I cannot say whether checking only the file in question fits better with clang-tidy’s policy or not.
Also, I’m not sure about ODR. Of course, it’s a possibility, but isn’t the programmer responsible for that? This will be more of an issue as soon as this check provides a Fix-It solution.

As for the other part, I've checked some guidelines (without any order or selection process)
MISRA C++: Don’t use #define, use const variables; Also don’t do math on enums
CppCoreGuidelines: Don’t use #define, use constexpr variables
SEI CERT C++: No mention of #define as far as I can tell
JSF AV C++: Don’t use #define, use const variables
HIC++: Don’t use #define, use const objects (reference to JSF AV C++ and MISRA C++)

So basically they're all the same. The only question is const vs constexpr

[AlexanderLanin]

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

86

Excluded (for now) due to string concatenation done by preprocessor when you write NAME1 NAME2 in your code. I've added a section into modernize-use-const-instead-of-define.rst to explain this.

[AlexanderLanin]

Applied review comments and added test cases regarding parenthesis, floats, doubles, wide chars etc

[aaron.ballman]

In D29692#676559, @AlexanderLanin wrote:

Thanks for the feedback. As I'm new to this I cannot say whether checking only the file in question fits better with clang-tidy’s policy or not.
Also, I’m not sure about ODR. Of course, it’s a possibility, but isn’t the programmer responsible for that? This will be more of an issue as soon as this check provides a Fix-It solution.

It's still an issue without the FixIt because the diagnostic guides the user to do something that may make their well-formed code into ill-formed (no diagnostic required!) code. I think the correct way to handle this is to ensure we don't diagnose macro definitions that exist in a header file, at least for C++ code.

As for the other part, I've checked some guidelines (without any order or selection process)
MISRA C++: Don’t use #define, use const variables; Also don’t do math on enums
CppCoreGuidelines: Don’t use #define, use constexpr variables
SEI CERT C++: No mention of #define as far as I can tell
JSF AV C++: Don’t use #define, use const variables
HIC++: Don’t use #define, use const objects (reference to JSF AV C++ and MISRA C++)

So basically they're all the same. The only question is const vs constexpr

I don't think the guidance in this check is incorrect, I just think the check is implemented too broadly. As for 'const' vs 'constexpr', such a decision may differ when compiling in C vs C++ mode.

[AlexanderLanin]

This is now detected by cppcoreguidelines-macro-usage, so this seems out of date.

clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
+  UseConstInsteadOfDefineCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseTransparentFunctorsCheck.cpp
   UseUsingCheck.cpp

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
+#include "UseConstInsteadOfDefineCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseTransparentFunctorsCheck.h"
 #include "UseUsingCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     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");
     CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");

clang-tidy/modernize/UseConstInsteadOfDefineCheck.h

+//===--- 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

clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp

+//===--- 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<UseConstInsteadOfDefineCheckPPCallbacks>(
+          &Compiler.getPreprocessor(), this));
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 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 `safety-no-assembler
   <http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html>`_ check
 
   Finds uses of inline assembler.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    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
    modernize-use-equals-delete
    modernize-use-nullptr
    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-using

docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst

+.. 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

test/clang-tidy/modernize-use-const-instead-of-define.cpp

+// 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)