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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
