From f547fc89c0730e0fee220d790957791bdfd23fcd Mon Sep 17 00:00:00 2001 From: Richard Date: Fri, 1 Apr 2022 13:53:00 -0600 Subject: [PATCH] [clang-tidy] Add modernize-macro-to-enum check [buildbot issues fixed] This check performs basic analysis of macros and replaces them with an anonymous unscoped enum. Using an unscoped anonymous enum ensures that everywhere the macro token was used previously, the enumerator name may be safely used. Potential macros for replacement must meet the following constraints: - Macros must expand only to integral literal tokens. The unary operators plus, minus and tilde are recognized to allow for positive, negative and bitwise negated integers. - Macros must be defined on sequential source file lines, or with only comment lines in between macro definitions. - Macros must all be defined in the same source file. - Macros must not be defined within a conditional compilation block. - Macros must not be defined adjacent to other preprocessor directives. - Macros must not be used in preprocessor conditions Each cluster of macros meeting the above constraints is presumed to be a set of values suitable for replacement by an anonymous enum. From there, a developer can give the anonymous enum a name and continue refactoring to a scoped enum if desired. Comments on the same line as a macro definition or between subsequent macro definitions are preserved in the output. No formatting is assumed in the provided replacements. The check cppcoreguidelines-macro-to-enum is an alias for this check. Fixes #27408 Differential Revision: https://reviews.llvm.org/D117522 --- .../clang-tidy/modernize/CMakeLists.txt | 1 + .../clang-tidy/modernize/MacroToEnumCheck.cpp | 489 ++++++++++++++++++ .../clang-tidy/modernize/MacroToEnumCheck.h | 34 ++ .../modernize/ModernizeTidyModule.cpp | 2 + clang-tools-extra/docs/ReleaseNotes.rst | 9 + .../cppcoreguidelines-macro-to-enum.rst | 9 + .../docs/clang-tidy/checks/list.rst | 2 + .../checks/modernize-macro-to-enum.rst | 66 +++ .../modernize-macro-to-enum.h | 25 + .../modernize-macro-to-enum2.h | 25 + .../modernize-macro-to-enum3.h | 20 + .../checkers/modernize-macro-to-enum.cpp | 239 +++++++++ 12 files changed, 921 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 147c2741d59e..6bf20552e018 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyModernizeModule DeprecatedIosBaseAliasesCheck.cpp LoopConvertCheck.cpp LoopConvertUtils.cpp + MacroToEnumCheck.cpp MakeSharedCheck.cpp MakeSmartPtrCheck.cpp MakeUniqueCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp new file mode 100644 index 000000000000..e6115af77bd5 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp @@ -0,0 +1,489 @@ +//===--- MacroToEnumCheck.cpp - clang-tidy --------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "MacroToEnumCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Preprocessor.h" +#include "llvm/ADT/STLExtras.h" +#include +#include +#include +#include + +namespace clang { +namespace tidy { +namespace modernize { + +static bool hasOnlyComments(SourceLocation Loc, const LangOptions &Options, + StringRef Text) { + // Use a lexer to look for tokens; if we find something other than a single + // hash, then there were intervening tokens between macro definitions. + std::string Buffer{Text}; + Lexer Lex(Loc, Options, Buffer.c_str(), Buffer.c_str(), + Buffer.c_str() + Buffer.size()); + Token Tok; + bool SeenHash = false; + while (!Lex.LexFromRawLexer(Tok)) { + if (Tok.getKind() == tok::hash && !SeenHash) { + SeenHash = true; + continue; + } + return false; + } + + // Everything in between was whitespace, so now just look for two blank lines, + // consisting of two consecutive EOL sequences, either '\n', '\r' or '\r\n'. + enum class WhiteSpace { + Nothing, + CR, + LF, + CRLF, + CRLFCR, + }; + + WhiteSpace State = WhiteSpace::Nothing; + for (char C : Text) { + switch (C) { + case '\r': + if (State == WhiteSpace::CR) + return false; + + State = State == WhiteSpace::CRLF ? WhiteSpace::CRLFCR : WhiteSpace::CR; + break; + + case '\n': + if (State == WhiteSpace::LF || State == WhiteSpace::CRLFCR) + return false; + + State = State == WhiteSpace::CR ? WhiteSpace::CRLF : WhiteSpace::LF; + break; + + default: + State = WhiteSpace::Nothing; + break; + } + } + + return true; +} + +// Validate that this literal token is a valid integer literal. A literal token +// could be a floating-point token, which isn't acceptable as a value for an +// enumeration. A floating-point token must either have a decimal point or an +// exponent ('E' or 'P'). +static bool isIntegralConstant(const Token &Token) { + const char *Begin = Token.getLiteralData(); + const char *End = Begin + Token.getLength(); + + // not a hexadecimal floating-point literal + if (Token.getLength() > 2 && Begin[0] == '0' && std::toupper(Begin[1]) == 'X') + return std::none_of(Begin + 2, End, [](char C) { + return C == '.' || std::toupper(C) == 'P'; + }); + + // not a decimal floating-point literal + return std::none_of( + Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; }); +} + +namespace { + +struct EnumMacro { + EnumMacro(Token Name, const MacroDirective *Directive) + : Name(Name), Directive(Directive) {} + + Token Name; + const MacroDirective *Directive; +}; + +using MacroList = SmallVector; + +enum class IncludeGuard { None, FileChanged, IfGuard, DefineGuard }; + +struct FileState { + FileState() + : ConditionScopes(0), LastLine(0), GuardScanner(IncludeGuard::None) {} + + int ConditionScopes; + unsigned int LastLine; + IncludeGuard GuardScanner; + SourceLocation LastMacroLocation; +}; + +class MacroToEnumCallbacks : public PPCallbacks { +public: + MacroToEnumCallbacks(MacroToEnumCheck *Check, const LangOptions &LangOptions, + const SourceManager &SM) + : Check(Check), LangOpts(LangOptions), SM(SM) {} + + void FileChanged(SourceLocation Loc, FileChangeReason Reason, + SrcMgr::CharacteristicKind FileType, + FileID PrevFID) override; + + void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, + StringRef FileName, bool IsAngled, + CharSourceRange FilenameRange, const FileEntry *File, + StringRef SearchPath, StringRef RelativePath, + const Module *Imported, + SrcMgr::CharacteristicKind FileType) override { + clearCurrentEnum(HashLoc); + } + + // Keep track of macro definitions that look like enums. + void MacroDefined(const Token &MacroNameTok, + const MacroDirective *MD) override; + + // Undefining an enum-like macro results in the enum set being dropped. + void MacroUndefined(const Token &MacroNameTok, const MacroDefinition &MD, + const MacroDirective *Undef) override; + + // Conditional compilation clears any adjacent enum-like macros. + // Macros used in conditional expressions clear any adjacent enum-like + // macros. + // Include guards are either + // #if !defined(GUARD) + // or + // #ifndef GUARD + void If(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue) override { + conditionStart(Loc); + checkCondition(ConditionRange); + } + void Ifndef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override { + conditionStart(Loc); + checkName(MacroNameTok); + } + void Ifdef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override { + conditionStart(Loc); + checkName(MacroNameTok); + } + void Elif(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue, SourceLocation IfLoc) override { + checkCondition(ConditionRange); + } + void Elifdef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override { + checkName(MacroNameTok); + } + void Elifdef(SourceLocation Loc, SourceRange ConditionRange, + SourceLocation IfLoc) override { + PPCallbacks::Elifdef(Loc, ConditionRange, IfLoc); + } + void Elifndef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override { + checkName(MacroNameTok); + } + void Elifndef(SourceLocation Loc, SourceRange ConditionRange, + SourceLocation IfLoc) override { + PPCallbacks::Elifndef(Loc, ConditionRange, IfLoc); + } + void Endif(SourceLocation Loc, SourceLocation IfLoc) override; + void PragmaDirective(SourceLocation Loc, + PragmaIntroducerKind Introducer) override; + + // After we've seen everything, issue warnings and fix-its. + void EndOfMainFile() override; + +private: + void newEnum() { + if (Enums.empty() || !Enums.back().empty()) + Enums.emplace_back(); + } + bool insideConditional() const { + return (CurrentFile->GuardScanner == IncludeGuard::DefineGuard && + CurrentFile->ConditionScopes > 1) || + (CurrentFile->GuardScanner != IncludeGuard::DefineGuard && + CurrentFile->ConditionScopes > 0); + } + bool isConsecutiveMacro(const MacroDirective *MD) const; + void rememberLastMacroLocation(const MacroDirective *MD) { + CurrentFile->LastLine = SM.getSpellingLineNumber(MD->getLocation()); + CurrentFile->LastMacroLocation = Lexer::getLocForEndOfToken( + MD->getMacroInfo()->getDefinitionEndLoc(), 0, SM, LangOpts); + } + void clearLastMacroLocation() { + CurrentFile->LastLine = 0; + CurrentFile->LastMacroLocation = SourceLocation{}; + } + void clearCurrentEnum(SourceLocation Loc); + void conditionStart(const SourceLocation &Loc); + void checkCondition(SourceRange ConditionRange); + void checkName(const Token &MacroNameTok); + void warnMacroEnum(const EnumMacro &Macro) const; + void fixEnumMacro(const MacroList &MacroList) const; + + MacroToEnumCheck *Check; + const LangOptions &LangOpts; + const SourceManager &SM; + SmallVector Enums; + SmallVector Files; + FileState *CurrentFile = nullptr; +}; + +bool MacroToEnumCallbacks::isConsecutiveMacro(const MacroDirective *MD) const { + if (CurrentFile->LastMacroLocation.isInvalid()) + return false; + + SourceLocation Loc = MD->getLocation(); + if (CurrentFile->LastLine + 1 == SM.getSpellingLineNumber(Loc)) + return true; + + SourceLocation Define = + SM.translateLineCol(SM.getFileID(Loc), SM.getSpellingLineNumber(Loc), 1); + CharSourceRange BetweenMacros{ + SourceRange{CurrentFile->LastMacroLocation, Define}, true}; + CharSourceRange CharRange = + Lexer::makeFileCharRange(BetweenMacros, SM, LangOpts); + StringRef BetweenText = Lexer::getSourceText(CharRange, SM, LangOpts); + return hasOnlyComments(Define, LangOpts, BetweenText); +} + +void MacroToEnumCallbacks::clearCurrentEnum(SourceLocation Loc) { + // Only drop the most recent Enum set if the directive immediately follows. + if (!Enums.empty() && !Enums.back().empty() && + SM.getSpellingLineNumber(Loc) == CurrentFile->LastLine + 1) + Enums.pop_back(); + + clearLastMacroLocation(); +} + +void MacroToEnumCallbacks::conditionStart(const SourceLocation &Loc) { + ++CurrentFile->ConditionScopes; + clearCurrentEnum(Loc); + if (CurrentFile->GuardScanner == IncludeGuard::FileChanged) + CurrentFile->GuardScanner = IncludeGuard::IfGuard; +} + +void MacroToEnumCallbacks::checkCondition(SourceRange Range) { + CharSourceRange CharRange = Lexer::makeFileCharRange( + CharSourceRange::getTokenRange(Range), SM, LangOpts); + std::string Text = Lexer::getSourceText(CharRange, SM, LangOpts).str(); + Lexer Lex(CharRange.getBegin(), LangOpts, Text.data(), Text.data(), + Text.data() + Text.size()); + Token Tok; + bool End = false; + while (!End) { + End = Lex.LexFromRawLexer(Tok); + if (Tok.is(tok::raw_identifier) && + Tok.getRawIdentifier().str() != "defined") + checkName(Tok); + } +} + +void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) { + std::string Id; + if (MacroNameTok.is(tok::raw_identifier)) + Id = MacroNameTok.getRawIdentifier().str(); + else if (MacroNameTok.is(tok::identifier)) + Id = MacroNameTok.getIdentifierInfo()->getName().str(); + else { + assert(false && "Expected either an identifier or raw identifier token"); + return; + } + + llvm::erase_if(Enums, [&Id](const MacroList &MacroList) { + return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) { + return Macro.Name.getIdentifierInfo()->getName().str() == Id; + }); + }); +} + +void MacroToEnumCallbacks::FileChanged(SourceLocation Loc, + FileChangeReason Reason, + SrcMgr::CharacteristicKind FileType, + FileID PrevFID) { + newEnum(); + if (Reason == EnterFile) { + Files.emplace_back(); + if (!SM.isInMainFile(Loc)) + Files.back().GuardScanner = IncludeGuard::FileChanged; + } else if (Reason == ExitFile) { + assert(CurrentFile->ConditionScopes == 0); + Files.pop_back(); + } + CurrentFile = &Files.back(); +} + +void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok, + const MacroDirective *MD) { + // Include guards are never candidates for becoming an enum. + if (CurrentFile->GuardScanner == IncludeGuard::IfGuard) { + CurrentFile->GuardScanner = IncludeGuard::DefineGuard; + return; + } + + if (insideConditional()) + return; + + if (SM.getFilename(MD->getLocation()).empty()) + return; + + const MacroInfo *Info = MD->getMacroInfo(); + if (Info->isFunctionLike() || Info->isBuiltinMacro() || + Info->tokens().empty() || Info->tokens().size() > 2) + return; + + // It can be +Lit, -Lit or just Lit. + Token Tok = Info->tokens().front(); + if (Info->tokens().size() == 2) { + if (!Tok.isOneOf(tok::TokenKind::minus, tok::TokenKind::plus, + tok::TokenKind::tilde)) + return; + Tok = Info->tokens().back(); + } + if (!Tok.isLiteral() || isStringLiteral(Tok.getKind()) || + !isIntegralConstant(Tok)) + return; + + if (!isConsecutiveMacro(MD)) + newEnum(); + Enums.back().emplace_back(MacroNameTok, MD); + rememberLastMacroLocation(MD); +} + +// Any macro that is undefined removes all adjacent macros from consideration as +// an enum and starts a new enum scan. +void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok, + const MacroDefinition &MD, + const MacroDirective *Undef) { + auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) { + return Macro.Name.getIdentifierInfo()->getName() == + MacroNameTok.getIdentifierInfo()->getName(); + }; + + auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) { + return llvm::any_of(MacroList, MatchesToken); + }); + if (It != Enums.end()) + Enums.erase(It); + + clearLastMacroLocation(); + CurrentFile->GuardScanner = IncludeGuard::None; +} + +void MacroToEnumCallbacks::Endif(SourceLocation Loc, SourceLocation IfLoc) { + // The if directive for the include guard isn't counted in the + // ConditionScopes. + if (CurrentFile->ConditionScopes == 0 && + CurrentFile->GuardScanner == IncludeGuard::DefineGuard) + return; + + // We don't need to clear the current enum because the start of the + // conditional block already took care of that. + assert(CurrentFile->ConditionScopes > 0); + --CurrentFile->ConditionScopes; +} + +namespace { + +template +bool textEquals(const char (&Needle)[N], const char *HayStack) { + return StringRef{HayStack, N - 1} == Needle; +} + +template size_t len(const char (&)[N]) { return N - 1; } + +} // namespace + +void MacroToEnumCallbacks::PragmaDirective(SourceLocation Loc, + PragmaIntroducerKind Introducer) { + if (CurrentFile->GuardScanner != IncludeGuard::FileChanged) + return; + + bool Invalid = false; + const char *Text = SM.getCharacterData( + Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts), &Invalid); + if (Invalid) + return; + + while (*Text && std::isspace(*Text)) + ++Text; + + if (textEquals("pragma", Text)) + return; + + Text += len("pragma"); + while (*Text && std::isspace(*Text)) + ++Text; + + if (textEquals("once", Text)) + CurrentFile->GuardScanner = IncludeGuard::IfGuard; +} + +void MacroToEnumCallbacks::EndOfMainFile() { + for (const MacroList &MacroList : Enums) { + if (MacroList.empty()) + continue; + + for (const EnumMacro &Macro : MacroList) + warnMacroEnum(Macro); + + fixEnumMacro(MacroList); + } +} + +void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const { + Check->diag(Macro.Directive->getLocation(), + "macro %0 defines an integral constant; prefer an enum instead") + << Macro.Name.getIdentifierInfo(); +} + +void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const { + SourceLocation Begin = + MacroList.front().Directive->getMacroInfo()->getDefinitionLoc(); + Begin = SM.translateLineCol(SM.getFileID(Begin), + SM.getSpellingLineNumber(Begin), 1); + DiagnosticBuilder Diagnostic = + Check->diag(Begin, "replace macro with enum") + << FixItHint::CreateInsertion(Begin, "enum {\n"); + + for (size_t I = 0u; I < MacroList.size(); ++I) { + const EnumMacro &Macro = MacroList[I]; + SourceLocation DefineEnd = + Macro.Directive->getMacroInfo()->getDefinitionLoc(); + SourceLocation DefineBegin = SM.translateLineCol( + SM.getFileID(DefineEnd), SM.getSpellingLineNumber(DefineEnd), 1); + CharSourceRange DefineRange; + DefineRange.setBegin(DefineBegin); + DefineRange.setEnd(DefineEnd); + Diagnostic << FixItHint::CreateRemoval(DefineRange); + + SourceLocation NameEnd = Lexer::getLocForEndOfToken( + Macro.Directive->getMacroInfo()->getDefinitionLoc(), 0, SM, LangOpts); + Diagnostic << FixItHint::CreateInsertion(NameEnd, " ="); + + SourceLocation ValueEnd = Lexer::getLocForEndOfToken( + Macro.Directive->getMacroInfo()->getDefinitionEndLoc(), 0, SM, + LangOpts); + if (I < MacroList.size() - 1) + Diagnostic << FixItHint::CreateInsertion(ValueEnd, ","); + } + + SourceLocation End = Lexer::getLocForEndOfToken( + MacroList.back().Directive->getMacroInfo()->getDefinitionEndLoc(), 0, SM, + LangOpts); + End = SM.translateLineCol(SM.getFileID(End), + SM.getSpellingLineNumber(End) + 1, 1); + Diagnostic << FixItHint::CreateInsertion(End, "};\n"); +} + +} // namespace + +void MacroToEnumCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks( + std::make_unique(this, getLangOpts(), SM)); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h new file mode 100644 index 000000000000..667ff49d671a --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h @@ -0,0 +1,34 @@ +//===--- MacroToEnumCheck.h - clang-tidy ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MACROTOENUMCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MACROTOENUMCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// Replaces groups of related macros with an unscoped anonymous enum. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-macro-to-enum.html +class MacroToEnumCheck : public ClangTidyCheck { +public: + MacroToEnumCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MACROTOENUMCHECK_H diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index d9ccd2cd0ad7..08076354dbe4 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -15,6 +15,7 @@ #include "DeprecatedHeadersCheck.h" #include "DeprecatedIosBaseAliasesCheck.h" #include "LoopConvertCheck.h" +#include "MacroToEnumCheck.h" #include "MakeSharedCheck.h" #include "MakeUniqueCheck.h" #include "PassByValueCheck.h" @@ -59,6 +60,7 @@ public: CheckFactories.registerCheck( "modernize-deprecated-ios-base-aliases"); CheckFactories.registerCheck("modernize-loop-convert"); + CheckFactories.registerCheck("modernize-macro-to-enum"); CheckFactories.registerCheck("modernize-make-shared"); CheckFactories.registerCheck("modernize-make-unique"); CheckFactories.registerCheck("modernize-pass-by-value"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 729bb3bbd736..e5140ed45cbb 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -106,9 +106,18 @@ New checks Finds initializations of C++ shared pointers to non-array type that are initialized with an array. +- New :doc:`modernize-macro-to-enum + ` check. + + Replaces groups of adjacent macros with an unscoped anonymous enum. + New check aliases ^^^^^^^^^^^^^^^^^ +- New alias :doc:`cppcoreguidelines-macro-to-enum + ` to :doc:`modernize-macro-to-enum + ` was added. + Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst new file mode 100644 index 000000000000..c9601c652ad9 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst @@ -0,0 +1,9 @@ +.. title:: clang-tidy - cppcoreguidelines-macro-to-enum +.. meta:: + :http-equiv=refresh: 5;URL=modernize-macro-to-enum.html + +cppcoreguidelines-macro-to-enum +=============================== + +The cppcoreguidelines-macro-to-enum check is an alias, please see +:doc:`modernize-macro-to-enum ` for more information. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index ebdcd7ae1041..e9367216ac51 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -234,6 +234,7 @@ Clang-Tidy Checks `modernize-deprecated-headers `_, "Yes" `modernize-deprecated-ios-base-aliases `_, "Yes" `modernize-loop-convert `_, "Yes" + `modernize-macro-to-enum `_, "Yes" `modernize-make-shared `_, "Yes" `modernize-make-unique `_, "Yes" `modernize-pass-by-value `_, "Yes" @@ -426,6 +427,7 @@ Clang-Tidy Checks `cppcoreguidelines-avoid-magic-numbers `_, `readability-magic-numbers `_, `cppcoreguidelines-c-copy-assignment-signature `_, `misc-unconventional-assign-operator `_, `cppcoreguidelines-explicit-virtual-functions `_, `modernize-use-override `_, "Yes" + `cppcoreguidelines-macro-to-enum `_, `modernize-macro-to-enum `_, "Yes" `cppcoreguidelines-non-private-member-variables-in-classes `_, `misc-non-private-member-variables-in-classes `_, `fuchsia-header-anon-namespaces `_, `google-build-namespaces `_, `google-readability-braces-around-statements `_, `readability-braces-around-statements `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst new file mode 100644 index 000000000000..6be406544ba8 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst @@ -0,0 +1,66 @@ +.. title:: clang-tidy - modernize-macro-to-enum + +modernize-macro-to-enum +======================= + +Replaces groups of adjacent macros with an unscoped anonymous enum. +Using an unscoped anonymous enum ensures that everywhere the macro +token was used previously, the enumerator name may be safely used. + +This check can be used to enforce the C++ core guideline `Enum.1: +Prefer enumerations over macros +`_, +within the constraints outlined below. + +Potential macros for replacement must meet the following constraints: + +- Macros must expand only to integral literal tokens. The unary operators + plus, minus and tilde are recognized to allow for positive, negative + and bitwise negated integers. More complicated integral constant + expressions are not currently recognized by this check. +- Macros must be defined on sequential source file lines, or with + only comment lines in between macro definitions. +- Macros must all be defined in the same source file. +- Macros must not be defined within a conditional compilation block. + (Conditional include guards are exempt from this constraint.) +- Macros must not be defined adjacent to other preprocessor directives. +- Macros must not be used in any conditional preprocessing directive. +- Macros must not be undefined. + +Each cluster of macros meeting the above constraints is presumed to +be a set of values suitable for replacement by an anonymous enum. +From there, a developer can give the anonymous enum a name and +continue refactoring to a scoped enum if desired. Comments on the +same line as a macro definition or between subsequent macro definitions +are preserved in the output. No formatting is assumed in the provided +replacements, although clang-tidy can optionally format all fixes. + +Examples: + +.. code-block:: c++ + + #define RED 0xFF0000 + #define GREEN 0x00FF00 + #define BLUE 0x0000FF + + #define TM_ONE 1 // Use tailored method one. + #define TM_TWO 2 // Use tailored method two. Method two + // is preferable to method one. + #define TM_THREE 3 // Use tailored method three. + +becomes + +.. code-block:: c++ + + enum { + RED = 0xFF0000, + GREEN = 0x00FF00, + BLUE = 0x0000FF + }; + + enum { + TM_ONE = 1, // Use tailored method one. + TM_TWO = 2, // Use tailored method two. Method two + // is preferable to method one. + TM_THREE = 3 // Use tailored method three. + }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h new file mode 100644 index 000000000000..f747be4761bb --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h @@ -0,0 +1,25 @@ +#if !defined(MODERNIZE_MACRO_TO_ENUM_H) +#define MODERNIZE_MACRO_TO_ENUM_H + +#include "modernize-macro-to-enum2.h" + +#define GG_RED 0xFF0000 +#define GG_GREEN 0x00FF00 +#define GG_BLUE 0x0000FF +// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_RED' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_GREEN' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_BLUE' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: GG_RED = 0xFF0000, +// CHECK-FIXES-NEXT: GG_GREEN = 0x00FF00, +// CHECK-FIXES-NEXT: GG_BLUE = 0x0000FF +// CHECK-FIXES-NEXT: }; + +#if 1 +#define RR_RED 1 +#define RR_GREEN 2 +#define RR_BLUE 3 +#endif + +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h new file mode 100644 index 000000000000..1616302a5b69 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h @@ -0,0 +1,25 @@ +#ifndef MODERNIZE_MACRO_TO_ENUM2_H +#define MODERNIZE_MACRO_TO_ENUM2_H + +#include "modernize-macro-to-enum3.h" + +#define GG2_RED 0xFF0000 +#define GG2_GREEN 0x00FF00 +#define GG2_BLUE 0x0000FF +// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG2_RED' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG2_GREEN' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG2_BLUE' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: GG2_RED = 0xFF0000, +// CHECK-FIXES-NEXT: GG2_GREEN = 0x00FF00, +// CHECK-FIXES-NEXT: GG2_BLUE = 0x0000FF +// CHECK-FIXES-NEXT: }; + +#if 1 +#define RR2_RED 1 +#define RR2_GREEN 2 +#define RR2_BLUE 3 +#endif + +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h new file mode 100644 index 000000000000..a9f1fb7e0fa4 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h @@ -0,0 +1,20 @@ +#pragma once + +#define GG3_RED 0xFF0000 +#define GG3_GREEN 0x00FF00 +#define GG3_BLUE 0x0000FF +// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG3_RED' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG3_GREEN' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG3_BLUE' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: GG3_RED = 0xFF0000, +// CHECK-FIXES-NEXT: GG3_GREEN = 0x00FF00, +// CHECK-FIXES-NEXT: GG3_BLUE = 0x0000FF +// CHECK-FIXES-NEXT: }; + +#if 1 +#define RR3_RED 1 +#define RR3_GREEN 2 +#define RR3_BLUE 3 +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp new file mode 100644 index 000000000000..5a5514fc074c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp @@ -0,0 +1,239 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum +// C++14 or later required for binary literals. + +#if 1 +#include "modernize-macro-to-enum.h" + +// These macros are skipped due to being inside a conditional compilation block. +#define GOO_RED 1 +#define GOO_GREEN 2 +#define GOO_BLUE 3 + +#endif + +#define RED 0xFF0000 +#define GREEN 0x00FF00 +#define BLUE 0x0000FF +// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum] +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: RED = 0xFF0000, +// CHECK-FIXES-NEXT: GREEN = 0x00FF00, +// CHECK-FIXES-NEXT: BLUE = 0x0000FF +// CHECK-FIXES-NEXT: }; + +// Verify that comments are preserved. +#define CoordModeOrigin 0 /* relative to the origin */ +#define CoordModePrevious 1 /* relative to previous point */ +// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: CoordModeOrigin = 0, /* relative to the origin */ +// CHECK-FIXES-NEXT: CoordModePrevious = 1 /* relative to previous point */ +// CHECK-FIXES-NEXT: }; + +// Verify that multiline comments are preserved. +#define BadDrawable 9 /* parameter not a Pixmap or Window */ +#define BadAccess 10 /* depending on context: + - key/button already grabbed + - attempt to free an illegal + cmap entry + - attempt to store into a read-only + color map entry. */ + // - attempt to modify the access control + // list from other than the local host. + // +#define BadAlloc 11 /* insufficient resources */ +// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: BadDrawable = 9, /* parameter not a Pixmap or Window */ +// CHECK-FIXES-NEXT: BadAccess = 10, /* depending on context: +// CHECK-FIXES-NEXT: - key/button already grabbed +// CHECK-FIXES-NEXT: - attempt to free an illegal +// CHECK-FIXES-NEXT: cmap entry +// CHECK-FIXES-NEXT: - attempt to store into a read-only +// CHECK-FIXES-NEXT: color map entry. */ +// CHECK-FIXES-NEXT: // - attempt to modify the access control +// CHECK-FIXES-NEXT: // list from other than the local host. +// CHECK-FIXES-NEXT: // +// CHECK-FIXES-NEXT: BadAlloc = 11 /* insufficient resources */ +// CHECK-FIXES-NEXT: }; + +// Undefining a macro invalidates adjacent macros +// from being considered as an enum. +#define REMOVED1 1 +#define REMOVED2 2 +#define REMOVED3 3 +#undef REMOVED2 +#define VALID1 1 +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: macro 'VALID1' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: VALID1 = 1 +// CHECK-FIXES-NEXT: }; + +#define UNDEF1 1 +#define UNDEF2 2 +#define UNDEF3 3 + +// Undefining a macro later invalidates the set of possible adjacent macros +// from being considered as an enum. +#undef UNDEF2 + +// Integral constants can have an optional sign +#define SIGNED1 +1 +#define SIGNED2 -1 +// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'SIGNED1' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'SIGNED2' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: SIGNED1 = +1, +// CHECK-FIXES-NEXT: SIGNED2 = -1 +// CHECK-FIXES-NEXT: }; + +// Integral constants with bitwise negated values +#define UNOP1 ~0U +#define UNOP2 ~1U +// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'UNOP1' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'UNOP2' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: UNOP1 = ~0U, +// CHECK-FIXES-NEXT: UNOP2 = ~1U +// CHECK-FIXES-NEXT: }; + +// Integral constants in other bases and with suffixes are OK +#define BASE1 0777 // octal +#define BASE2 0xDEAD // hexadecimal +#define BASE3 0b0011 // binary +#define SUFFIX1 +1U +#define SUFFIX2 -1L +#define SUFFIX3 +1UL +#define SUFFIX4 -1LL +#define SUFFIX5 +1ULL +// CHECK-MESSAGES: :[[@LINE-8]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'BASE1' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'BASE2' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'BASE3' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'SUFFIX1' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'SUFFIX2' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'SUFFIX3' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'SUFFIX4' defines an integral constant; prefer an enum instead +// CHECK-MESSAGES: :[[@LINE-9]]:9: warning: macro 'SUFFIX5' defines an integral constant; prefer an enum instead +// CHECK-FIXES: enum { +// CHECK-FIXES-NEXT: BASE1 = 0777, // octal +// CHECK-FIXES-NEXT: BASE2 = 0xDEAD, // hexadecimal +// CHECK-FIXES-NEXT: BASE3 = 0b0011, // binary +// CHECK-FIXES-NEXT: SUFFIX1 = +1U, +// CHECK-FIXES-NEXT: SUFFIX2 = -1L, +// CHECK-FIXES-NEXT: SUFFIX3 = +1UL, +// CHECK-FIXES-NEXT: SUFFIX4 = -1LL, +// CHECK-FIXES-NEXT: SUFFIX5 = +1ULL +// CHECK-FIXES-NEXT: }; + +// Macros appearing in conditional expressions can't be replaced +// by enums. +#define USE_FOO 1 +#define USE_BAR 0 +#define USE_IF 1 +#define USE_ELIF 1 +#define USE_IFDEF 1 +#define USE_IFNDEF 1 + +#if defined(USE_FOO) && USE_FOO +extern void foo(); +#else +inline void foo() {} +#endif + +#if USE_BAR +extern void bar(); +#else +inline void bar() {} +#endif + +#if USE_IF +inline void used_if() {} +#endif + +#if 0 +#elif USE_ELIF +inline void used_elif() {} +#endif + +#ifdef USE_IFDEF +inline void used_ifdef() {} +#endif + +#ifndef USE_IFNDEF +#else +inline void used_ifndef() {} +#endif + +// Regular conditional compilation blocks should leave previous +// macro enums alone. +#if 0 +#include +#endif + +// Conditional compilation blocks invalidate adjacent macros +// from being considered as an enum. Conditionally compiled +// blocks could contain macros that should rightly be included +// in the enum, but we can't explore multiple branches of a +// conditionally compiled section in clang-tidy, only the active +// branch based on compilation options. +#define CONDITION1 1 +#define CONDITION2 2 +#if 0 +#define CONDITION3 3 +#else +#define CONDITION3 -3 +#endif + +#define IFDEF1 1 +#define IFDEF2 2 +#ifdef FROB +#define IFDEF3 3 +#endif + +#define IFNDEF1 1 +#define IFNDEF2 2 +#ifndef GOINK +#define IFNDEF3 3 +#endif + +// These macros do not expand to integral constants. +#define HELLO "Hello, " +#define WORLD "World" +#define EPS1 1.0F +#define EPS2 1e5 +#define EPS3 1. + +#define DO_RED draw(RED) +#define DO_GREEN draw(GREEN) +#define DO_BLUE draw(BLUE) + +#define FN_RED(x) draw(RED | x) +#define FN_GREEN(x) draw(GREEN | x) +#define FN_BLUE(x) draw(BLUE | x) + +extern void draw(unsigned int Color); + +void f() +{ + draw(RED); + draw(GREEN); + draw(BLUE); + DO_RED; + DO_GREEN; + DO_BLUE; + FN_RED(0); + FN_GREEN(0); + FN_BLUE(0); +}