[clang-tools-extra] d83ecd7 - [clang-tidy] Narrow cppguidelines-macro-usage to actual constants
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 19 11:29:12 PST 2022
Author: Richard
Date: 2022-01-19T12:28:22-07:00
New Revision: d83ecd77cc0f16cb5fbabe03d37829893ac8b56d
URL: https://github.com/llvm/llvm-project/commit/d83ecd77cc0f16cb5fbabe03d37829893ac8b56d
DIFF: https://github.com/llvm/llvm-project/commit/d83ecd77cc0f16cb5fbabe03d37829893ac8b56d.diff
LOG: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants
Previously, any macro that didn't look like a varargs macro
or a function style macro was reported with a warning that
it should be replaced with a constexpr const declaration.
This is only reasonable when the macro body contains constants
and not expansions like ",", "[[noreturn]]", "__declspec(xxx)",
etc.
So instead of always issuing a warning about every macro that
doesn't look like a varargs or function style macro, examine the
tokens in the macro and only warn about the macro if it contains
only comment and constant tokens.
Differential Revision: https://reviews.llvm.org/D116386
Fixes #39945
Added:
Modified:
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
index a6f6ca4c1abd6..94a646c7fca03 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -14,25 +14,25 @@
#include "llvm/Support/Regex.h"
#include <algorithm>
#include <cctype>
+#include <functional>
namespace clang {
namespace tidy {
namespace cppcoreguidelines {
-namespace {
-
-bool isCapsOnly(StringRef Name) {
- return std::all_of(Name.begin(), Name.end(), [](const char C) {
- if (std::isupper(C) || std::isdigit(C) || C == '_')
- return true;
- return false;
+static bool isCapsOnly(StringRef Name) {
+ return llvm::all_of(Name, [](const char C) {
+ return std::isupper(C) || std::isdigit(C) || C == '_';
});
}
+namespace {
+
class MacroUsageCallbacks : public PPCallbacks {
public:
MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM,
- StringRef RegExpStr, bool CapsOnly, bool IgnoreCommandLine)
+ StringRef RegExpStr, bool CapsOnly,
+ bool IgnoreCommandLine)
: Check(Check), SM(SM), RegExp(RegExpStr), CheckCapsOnly(CapsOnly),
IgnoreCommandLineMacros(IgnoreCommandLine) {}
void MacroDefined(const Token &MacroNameTok,
@@ -79,21 +79,24 @@ void MacroUsageCheck::registerPPCallbacks(const SourceManager &SM,
}
void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
- StringRef Message =
- "macro '%0' used to declare a constant; consider using a 'constexpr' "
- "constant";
-
- /// A variadic macro is function-like at the same time. Therefore variadic
- /// macros are checked first and will be excluded for the function-like
- /// diagnostic.
- if (MD->getMacroInfo()->isVariadic())
+ const MacroInfo *Info = MD->getMacroInfo();
+ StringRef Message;
+
+ if (llvm::all_of(Info->tokens(), std::mem_fn(&Token::isLiteral)))
+ Message = "macro '%0' used to declare a constant; consider using a "
+ "'constexpr' constant";
+ // A variadic macro is function-like at the same time. Therefore variadic
+ // macros are checked first and will be excluded for the function-like
+ // diagnostic.
+ else if (Info->isVariadic())
Message = "variadic macro '%0' used; consider using a 'constexpr' "
"variadic template function";
- else if (MD->getMacroInfo()->isFunctionLike())
+ else if (Info->isFunctionLike())
Message = "function-like macro '%0' used; consider a 'constexpr' template "
"function";
- diag(MD->getLocation(), Message) << MacroName;
+ if (!Message.empty())
+ diag(MD->getLocation(), Message) << MacroName;
}
void MacroUsageCheck::warnNaming(const MacroDirective *MD,
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 098edd90b725f..683e914b7e863 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -83,6 +83,9 @@ Improvements to clang-tidy
- Generalized the `modernize-use-default-member-init` check to handle non-default
constructors.
+- Eliminated false positives for `cppcoreguidelines-macro-usage` by restricting
+ the warning about using constants to only macros that expand to literals.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
index 29e60cc88fa2f..ca7e54429c236 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -7,10 +7,40 @@ Finds macro usage that is considered problematic because better language
constructs exist for the task.
The relevant sections in the C++ Core Guidelines are
-`Enum.1 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enum1-prefer-enumerations-over-macros>`_,
-`ES.30 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es30-dont-use-macros-for-program-text-manipulation>`_,
-`ES.31 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions>`_ and
-`ES.33 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es33-if-you-must-use-macros-give-them-unique-names>`_.
+`ES.31 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions>`_, and
+`ES.32 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es32-use-all_caps-for-all-macro-names>`_.
+
+Examples:
+
+.. code-block:: c++
+
+ #define C 0
+ #define F1(x, y) ((a) > (b) ? (a) : (b))
+ #define F2(...) (__VA_ARGS__)
+ #define COMMA ,
+ #define NORETURN [[noreturn]]
+ #define DEPRECATED attribute((deprecated))
+ #if LIB_EXPORTS
+ #define DLLEXPORTS __declspec(dllexport)
+ #else
+ #define DLLEXPORTS __declspec(dllimport)
+ #endif
+
+results in the following warnings:
+
+.. code-block:: c++
+
+ 4 warnings generated.
+ test.cpp:1:9: warning: macro 'C' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]
+ #define C 0
+ ^
+ test.cpp:2:9: warning: function-like macro 'F1' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
+ #define F1(x, y) ((a) > (b) ? (a) : (b))
+ ^
+ test.cpp:3:9: warning: variadic macro 'F2' used; consider using a 'constexpr' variadic template function [cppcoreguidelines-macro-usage]
+ #define F2(...) (__VA_ARGS__)
+ ^
+
Options
-------
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
index edce328ec65a9..404aafb6b1c45 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
#ifndef INCLUDE_GUARD
#define INCLUDE_GUARD
@@ -6,6 +6,21 @@
#define PROBLEMATIC_CONSTANT 0
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
@@ -15,4 +30,17 @@
#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
#endif
More information about the cfe-commits
mailing list