https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/86129
>From 4e0845a143a820d4a68ffbdced206654c7593359 Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Fri, 15 Mar 2024 08:07:47 +0800 Subject: [PATCH 1/7] [clang-tidy] add new check readability-enum-initial-value Fixes: #85243. --- .../clang-tidy/readability/CMakeLists.txt | 1 + .../readability/EnumInitialValueCheck.cpp | 82 +++++++++++++++++++ .../readability/EnumInitialValueCheck.h | 31 +++++++ .../readability/ReadabilityTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ .../docs/clang-tidy/checks/list.rst | 1 + .../checks/readability/enum-initial-value.rst | 45 ++++++++++ .../checkers/readability/enum-initial-value.c | 27 ++++++ .../readability/enum-initial-value.cpp | 27 ++++++ 9 files changed, 223 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 5728c9970fb65d..dd772d69202548 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -17,6 +17,7 @@ add_clang_library(clangTidyReadabilityModule DeleteNullPointerCheck.cpp DuplicateIncludeCheck.cpp ElseAfterReturnCheck.cpp + EnumInitialValueCheck.cpp FunctionCognitiveComplexityCheck.cpp FunctionSizeCheck.cpp IdentifierLengthCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp new file mode 100644 index 00000000000000..78d5101d439dde --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -0,0 +1,82 @@ +//===--- EnumInitialValueCheck.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 "EnumInitialValueCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/Decl.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "llvm/ADT/SmallString.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(EnumDecl, isNoneEnumeratorsInitialized) { + return llvm::all_of(Node.enumerators(), [](EnumConstantDecl const *ECD) { + return ECD->getInitExpr() == nullptr; + }); +} + +AST_MATCHER(EnumDecl, isOnlyFirstEnumeratorsInitialized) { + for (EnumConstantDecl const *ECD : Node.enumerators()) + if (ECD == *Node.enumerator_begin()) { + if (ECD->getInitExpr() == nullptr) + return false; + } else { + if (ECD->getInitExpr() != nullptr) + return false; + } + return true; +} + +AST_MATCHER(EnumDecl, isAllEnumeratorsInitialized) { + return llvm::all_of(Node.enumerators(), [](EnumConstantDecl const *ECD) { + return ECD->getInitExpr() != nullptr; + }); +} + +} // namespace + +void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(enumDecl(unless(anyOf(isNoneEnumeratorsInitialized(), + isOnlyFirstEnumeratorsInitialized(), + isAllEnumeratorsInitialized()))) + .bind("enum"), + this); +} + +void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("enum"); + assert(Enum != nullptr); + SourceLocation Loc = Enum->getBeginLoc(); + if (Loc.isInvalid() || Loc.isMacroID()) + return; + DiagnosticBuilder Diag = + diag(Loc, "inital value in enum %0 has readability issue, " + "explicit initialization of all of enumerators") + << Enum->getName(); + for (EnumConstantDecl const *ECD : Enum->enumerators()) + if (ECD->getInitExpr() == nullptr) { + SourceLocation ECDLoc = ECD->getEndLoc(); + if (ECDLoc.isInvalid() || ECDLoc.isMacroID()) + continue; + std::optional<Token> Next = utils::lexer::findNextTokenSkippingComments( + ECDLoc, *Result.SourceManager, getLangOpts()); + if (!Next.has_value() || Next->getLocation().isMacroID()) + continue; + llvm::SmallString<8> Str{" = "}; + ECD->getInitVal().toString(Str); + Diag << FixItHint::CreateInsertion(Next->getLocation(), Str); + } +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h new file mode 100644 index 00000000000000..6b4e0e28e35be0 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h @@ -0,0 +1,31 @@ +//===--- EnumInitialValueCheck.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_READABILITY_ENUMINITIALVALUECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ENUMINITIALVALUECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::readability { + +/// Detects explicit initialization of a part of enumerators in an enumeration, and +/// relying on compiler to initialize the others. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability/enum-initial-value.html +class EnumInitialValueCheck : public ClangTidyCheck { +public: + EnumInitialValueCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ENUMINITIALVALUECHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index bca2c425111f6c..376b84683df74e 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -22,6 +22,7 @@ #include "DeleteNullPointerCheck.h" #include "DuplicateIncludeCheck.h" #include "ElseAfterReturnCheck.h" +#include "EnumInitialValueCheck.h" #include "FunctionCognitiveComplexityCheck.h" #include "FunctionSizeCheck.h" #include "IdentifierLengthCheck.h" @@ -92,6 +93,8 @@ class ReadabilityModule : public ClangTidyModule { "readability-duplicate-include"); CheckFactories.registerCheck<ElseAfterReturnCheck>( "readability-else-after-return"); + CheckFactories.registerCheck<EnumInitialValueCheck>( + "readability-enum-initial-value"); CheckFactories.registerCheck<FunctionCognitiveComplexityCheck>( "readability-function-cognitive-complexity"); CheckFactories.registerCheck<FunctionSizeCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 44680f79de6f54..53ce4acad90d52 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -116,6 +116,12 @@ New checks Finds initializer lists for aggregate types that could be written as designated initializers instead. +- New :doc:`readability-enum-initial-value + <clang-tidy/checks/readability/enum-initial-value>` check. + + Detects explicit initialization of a part of enumerators in an enumeration, and + relying on compiler to initialize the others. + - New :doc:`readability-use-std-min-max <clang-tidy/checks/readability/use-std-min-max>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index d03e7af688f007..727489c8de65f4 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -351,6 +351,7 @@ Clang-Tidy Checks :doc:`readability-delete-null-pointer <readability/delete-null-pointer>`, "Yes" :doc:`readability-duplicate-include <readability/duplicate-include>`, "Yes" :doc:`readability-else-after-return <readability/else-after-return>`, "Yes" + :doc:`readability-enum-initial-value <readability/enum-initial-value>`, "Yes" :doc:`readability-function-cognitive-complexity <readability/function-cognitive-complexity>`, :doc:`readability-function-size <readability/function-size>`, :doc:`readability-identifier-length <readability/identifier-length>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst new file mode 100644 index 00000000000000..f6292a0933aa60 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst @@ -0,0 +1,45 @@ +.. title:: clang-tidy - readability-enum-initial-value + +readability-enum-initial-value +============================== + +Detects explicit initialization of a part of enumerators in an enumeration, and +relying on compiler to initialize the others. + +It causes readability issues and reduces the maintainability. When adding new +enumerations, the developers need to be careful for potiential enumeration value +conflicts. + +In an enumeration, the following three cases are accepted. +1. none of enumerators are explicit initialized. +2. the first enumerator is explicit initialized. +3. all of enumerators are explicit initialized. + +.. code-block:: c++ + // valid, none of enumerators are initialized. + enum A { + e0, + e1, + e2, + }; + + // valid, the first enumerator is initialized. + enum A { + e0 = 0, + e1, + e2, + }; + + // valid, all of enumerators are initialized. + enum A { + e0 = 0, + e1 = 1, + e2 = 2, + }; + + // invalid, e1 is not explicit initialized. + enum A { + e0 = 0, + e1, + e2 = 2, + }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c new file mode 100644 index 00000000000000..bb8bdf9e709f2a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c @@ -0,0 +1,27 @@ +// RUN: %check_clang_tidy %s readability-enum-initial-value %t + +enum EError { + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital value in enum EError has readability issue, explicit initialization of all of enumerators + EError_a = 1, + EError_b, + // CHECK-FIXES: EError_b = 2, + EError_c = 3, +}; + +enum ENone { + ENone_a, + ENone_b, + eENone_c, +}; + +enum EFirst { + EFirst_a = 1, + EFirst_b, + EFirst_c, +}; + +enum EAll { + EAll_a = 1, + EAll_b = 2, + EAll_c = 3, +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp new file mode 100644 index 00000000000000..e95b3a6d8d675a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp @@ -0,0 +1,27 @@ +// RUN: %check_clang_tidy %s readability-enum-initial-value %t + +enum class EError { + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital value in enum EError has readability issue, explicit initialization of all of enumerators + EError_a = 1, + EError_b, + // CHECK-FIXES: EError_b = 2, + EError_c = 3, +}; + +enum class ENone { + ENone_a, + ENone_b, + eENone_c, +}; + +enum class EFirst { + EFirst_a = 1, + EFirst_b, + EFirst_c, +}; + +enum class EAll { + EAll_a = 1, + EAll_b = 2, + EAll_c = 3, +}; >From d1496618cb7f36e2047920b1e91d2f5c74c4ba87 Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Thu, 21 Mar 2024 23:11:41 +0800 Subject: [PATCH 2/7] fix format --- .../clang-tidy/readability/EnumInitialValueCheck.h | 4 ++-- .../docs/clang-tidy/checks/readability/enum-initial-value.rst | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h index 6b4e0e28e35be0..8a0ddd2b387c74 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h @@ -13,8 +13,8 @@ namespace clang::tidy::readability { -/// Detects explicit initialization of a part of enumerators in an enumeration, and -/// relying on compiler to initialize the others. +/// Detects explicit initialization of a part of enumerators in an enumeration, +/// and relying on compiler to initialize the others. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/readability/enum-initial-value.html diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst index f6292a0933aa60..57467c8420a5dc 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst @@ -16,6 +16,7 @@ In an enumeration, the following three cases are accepted. 3. all of enumerators are explicit initialized. .. code-block:: c++ + // valid, none of enumerators are initialized. enum A { e0, >From 089815ca736ec93873c1433d89fc29fe7caf8232 Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Fri, 22 Mar 2024 13:21:23 +0800 Subject: [PATCH 3/7] fix msg --- .../clang-tidy/readability/EnumInitialValueCheck.cpp | 4 ++-- .../clang-tidy/readability/EnumInitialValueCheck.h | 3 +++ .../test/clang-tidy/checkers/readability/enum-initial-value.c | 2 +- .../clang-tidy/checkers/readability/enum-initial-value.cpp | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp index 78d5101d439dde..02c74dbb3e71fc 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -61,8 +61,8 @@ void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { if (Loc.isInvalid() || Loc.isMacroID()) return; DiagnosticBuilder Diag = - diag(Loc, "inital value in enum %0 has readability issue, " - "explicit initialization of all of enumerators") + diag(Loc, "inital values in enum %0 are not consistent, consider " + "explicit initialization first, all or none of enumerators") << Enum->getName(); for (EnumConstantDecl const *ECD : Enum->enumerators()) if (ECD->getInitExpr() == nullptr) { diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h index 8a0ddd2b387c74..ac1ac275df8b8c 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h @@ -24,6 +24,9 @@ class EnumInitialValueCheck : public ClangTidyCheck { : ClangTidyCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c index bb8bdf9e709f2a..308d2e232a1428 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c @@ -1,7 +1,7 @@ // RUN: %check_clang_tidy %s readability-enum-initial-value %t enum EError { - // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital value in enum EError has readability issue, explicit initialization of all of enumerators + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum EError are not consistent EError_a = 1, EError_b, // CHECK-FIXES: EError_b = 2, diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp index e95b3a6d8d675a..f3a163dfb2a954 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp @@ -1,7 +1,7 @@ // RUN: %check_clang_tidy %s readability-enum-initial-value %t enum class EError { - // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital value in enum EError has readability issue, explicit initialization of all of enumerators + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum EError are not consistent EError_a = 1, EError_b, // CHECK-FIXES: EError_b = 2, >From eaeb4916cf051635cf7ee12a38d8f5d08bd7350a Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Tue, 26 Mar 2024 21:41:51 +0800 Subject: [PATCH 4/7] fix --- .../readability/EnumInitialValueCheck.cpp | 34 +++++++++++-------- .../checks/readability/enum-initial-value.rst | 5 ++- .../checkers/readability/enum-initial-value.c | 21 ++++++++++-- .../readability/enum-initial-value.cpp | 4 +-- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp index 02c74dbb3e71fc..f6e32fed4fc01b 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -20,15 +20,17 @@ namespace clang::tidy::readability { namespace { -AST_MATCHER(EnumDecl, isNoneEnumeratorsInitialized) { - return llvm::all_of(Node.enumerators(), [](EnumConstantDecl const *ECD) { +bool isNoneEnumeratorsInitialized(const EnumDecl &Node) { + return llvm::all_of(Node.enumerators(), [](const EnumConstantDecl *ECD) { return ECD->getInitExpr() == nullptr; }); } -AST_MATCHER(EnumDecl, isOnlyFirstEnumeratorsInitialized) { - for (EnumConstantDecl const *ECD : Node.enumerators()) - if (ECD == *Node.enumerator_begin()) { +bool isOnlyFirstEnumeratorsInitialized(const EnumDecl &Node) { + bool IsFirst = true; + for (const EnumConstantDecl *ECD : Node.enumerators()) + if (IsFirst) { + IsFirst = false; if (ECD->getInitExpr() == nullptr) return false; } else { @@ -38,33 +40,35 @@ AST_MATCHER(EnumDecl, isOnlyFirstEnumeratorsInitialized) { return true; } -AST_MATCHER(EnumDecl, isAllEnumeratorsInitialized) { - return llvm::all_of(Node.enumerators(), [](EnumConstantDecl const *ECD) { +bool isAllEnumeratorsInitialized(const EnumDecl &Node) { + return llvm::all_of(Node.enumerators(), [](const EnumConstantDecl *ECD) { return ECD->getInitExpr() != nullptr; }); } +AST_MATCHER(EnumDecl, hasMeaningfulInitialValues) { + return isNoneEnumeratorsInitialized(Node) || + isOnlyFirstEnumeratorsInitialized(Node) || + isAllEnumeratorsInitialized(Node); +} + } // namespace void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(enumDecl(unless(anyOf(isNoneEnumeratorsInitialized(), - isOnlyFirstEnumeratorsInitialized(), - isAllEnumeratorsInitialized()))) - .bind("enum"), - this); + Finder->addMatcher( + enumDecl(unless(hasMeaningfulInitialValues())).bind("enum"), this); } void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("enum"); - assert(Enum != nullptr); SourceLocation Loc = Enum->getBeginLoc(); if (Loc.isInvalid() || Loc.isMacroID()) return; DiagnosticBuilder Diag = diag(Loc, "inital values in enum %0 are not consistent, consider " "explicit initialization first, all or none of enumerators") - << Enum->getName(); - for (EnumConstantDecl const *ECD : Enum->enumerators()) + << Enum; + for (const EnumConstantDecl *ECD : Enum->enumerators()) if (ECD->getInitExpr() == nullptr) { SourceLocation ECDLoc = ECD->getEndLoc(); if (ECDLoc.isInvalid() || ECDLoc.isMacroID()) diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst index 57467c8420a5dc..1c16a365255cba 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst @@ -6,9 +6,8 @@ readability-enum-initial-value Detects explicit initialization of a part of enumerators in an enumeration, and relying on compiler to initialize the others. -It causes readability issues and reduces the maintainability. When adding new -enumerations, the developers need to be careful for potiential enumeration value -conflicts. +When adding new enumerations, inconsistent initial value will cause potential +enumeration value conflicts. In an enumeration, the following three cases are accepted. 1. none of enumerators are explicit initialized. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c index 308d2e232a1428..4b87183e480603 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c @@ -1,7 +1,7 @@ // RUN: %check_clang_tidy %s readability-enum-initial-value %t enum EError { - // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum EError are not consistent + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EError' are not consistent EError_a = 1, EError_b, // CHECK-FIXES: EError_b = 2, @@ -11,7 +11,7 @@ enum EError { enum ENone { ENone_a, ENone_b, - eENone_c, + EENone_c, }; enum EFirst { @@ -25,3 +25,20 @@ enum EAll { EAll_b = 2, EAll_c = 3, }; + +#define ENUMERATOR_1 EMacro1_b +enum EMacro1 { + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EMacro1' are not consistent + EMacro1_a = 1, + ENUMERATOR_1, + EMacro1_c = 3, +}; + + +#define ENUMERATOR_2 EMacro2_b = 2 +enum EMacro2 { + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EMacro2' are not consistent + EMacro2_a = 1, + ENUMERATOR_2, + EMacro2_c, +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp index f3a163dfb2a954..3c4ba970372a07 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp @@ -1,7 +1,7 @@ // RUN: %check_clang_tidy %s readability-enum-initial-value %t enum class EError { - // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum EError are not consistent + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EError' are not consistent EError_a = 1, EError_b, // CHECK-FIXES: EError_b = 2, @@ -11,7 +11,7 @@ enum class EError { enum class ENone { ENone_a, ENone_b, - eENone_c, + EENone_c, }; enum class EFirst { >From 02fd17b13d5d3270d5b5cc4c9088b7dcf7024edd Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Tue, 26 Mar 2024 23:30:45 +0800 Subject: [PATCH 5/7] extend check --- .../readability/EnumInitialValueCheck.cpp | 153 +++++++++++++++--- .../readability/EnumInitialValueCheck.h | 8 +- .../checks/readability/enum-initial-value.rst | 30 ++++ .../checkers/readability/enum-initial-value.c | 38 ++++- 4 files changed, 203 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp index f6e32fed4fc01b..119e59cd721a14 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -12,12 +12,29 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" using namespace clang::ast_matchers; namespace clang::tidy::readability { +EnumInitialValueCheck::EnumInitialValueCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowExplicitZeroFirstInitialValue( + Options.get("AllowExplicitZeroFirstInitialValue", true)), + AllowExplicitLinearInitialValues( + Options.get("AllowExplicitLinearInitialValues", true)) {} + +void EnumInitialValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowExplicitZeroFirstInitialValue", + AllowExplicitZeroFirstInitialValue); + Options.store(Opts, "AllowExplicitLinearInitialValues", + AllowExplicitLinearInitialValues); +} + namespace { bool isNoneEnumeratorsInitialized(const EnumDecl &Node) { @@ -37,7 +54,7 @@ bool isOnlyFirstEnumeratorsInitialized(const EnumDecl &Node) { if (ECD->getInitExpr() != nullptr) return false; } - return true; + return !IsFirst; } bool isAllEnumeratorsInitialized(const EnumDecl &Node) { @@ -46,41 +63,131 @@ bool isAllEnumeratorsInitialized(const EnumDecl &Node) { }); } -AST_MATCHER(EnumDecl, hasMeaningfulInitialValues) { +/// Check if \p Enumerator is initialized with a (potentially negated) \c +/// IntegerLiteral. +bool isInitializedByLiteral(const EnumConstantDecl *Enumerator) { + const Expr *const Init = Enumerator->getInitExpr(); + if (!Init) + return false; + return Init->isIntegerConstantExpr(Enumerator->getASTContext()); +} + +AST_MATCHER(EnumDecl, hasConsistentInitialValues) { return isNoneEnumeratorsInitialized(Node) || isOnlyFirstEnumeratorsInitialized(Node) || isAllEnumeratorsInitialized(Node); } +AST_MATCHER(EnumDecl, hasZeroFirstInitialValue) { + if (Node.enumerators().empty()) + return false; + const EnumConstantDecl *ECD = *Node.enumerators().begin(); + return isOnlyFirstEnumeratorsInitialized(Node) && + isInitializedByLiteral(ECD) && ECD->getInitVal().isZero(); +} + +/// Excludes bitfields because enumerators initialized with the result of a +/// bitwise operator on enumeration values or any other expr that is not a +/// potentially negative integer literal. +/// Enumerations where it is not directly clear if they are used with +/// bitmask, evident when enumerators are only initialized with (potentially +/// negative) integer literals, are ignored. This is also the case when all +/// enumerators are powers of two (e.g., 0, 1, 2). +AST_MATCHER(EnumDecl, hasLinearInitialValues) { + if (Node.enumerators().empty()) + return false; + const EnumConstantDecl *const FirstEnumerator = *Node.enumerator_begin(); + llvm::APSInt PrevValue = FirstEnumerator->getInitVal(); + if (!isInitializedByLiteral(FirstEnumerator)) + return false; + bool AllEnumeratorsArePowersOfTwo = true; + for (const EnumConstantDecl *Enumerator : + llvm::drop_begin(Node.enumerators())) { + const llvm::APSInt NewValue = Enumerator->getInitVal(); + if (NewValue != ++PrevValue) + return false; + if (!isInitializedByLiteral(Enumerator)) + return false; + PrevValue = NewValue; + AllEnumeratorsArePowersOfTwo &= NewValue.isPowerOf2(); + } + return !AllEnumeratorsArePowersOfTwo; +} + } // namespace void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - enumDecl(unless(hasMeaningfulInitialValues())).bind("enum"), this); + enumDecl(unless(hasConsistentInitialValues())).bind("inconsistent"), + this); + if (!AllowExplicitZeroFirstInitialValue) + Finder->addMatcher(enumDecl(hasZeroFirstInitialValue()).bind("zero_first"), + this); + if (!AllowExplicitLinearInitialValues) + Finder->addMatcher(enumDecl(hasLinearInitialValues()).bind("linear"), this); +} + +static void cleanInitialValue(DiagnosticBuilder &Diag, + const EnumConstantDecl *ECD, + const SourceManager &SM, + const LangOptions &LangOpts) { + std::optional<Token> EqualToken = utils::lexer::findNextTokenSkippingComments( + ECD->getLocation(), SM, LangOpts); + if (!EqualToken.has_value()) + return; + SourceLocation EqualLoc{EqualToken->getLocation()}; + if (EqualLoc.isInvalid() || EqualLoc.isMacroID()) + return; + SourceRange InitExprRange = ECD->getInitExpr()->getSourceRange(); + if (InitExprRange.isInvalid() || InitExprRange.getBegin().isMacroID() || + InitExprRange.getEnd().isMacroID()) + return; + Diag << FixItHint::CreateRemoval(EqualLoc) + << FixItHint::CreateRemoval(InitExprRange); + return; } void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("enum"); - SourceLocation Loc = Enum->getBeginLoc(); - if (Loc.isInvalid() || Loc.isMacroID()) + if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("inconsistent")) { + SourceLocation Loc = Enum->getBeginLoc(); + if (Loc.isInvalid() || Loc.isMacroID()) + return; + DiagnosticBuilder Diag = + diag(Loc, "inital values in enum %0 are not consistent, consider " + "explicit initialization first, all or none of enumerators") + << Enum; + for (const EnumConstantDecl *ECD : Enum->enumerators()) + if (ECD->getInitExpr() == nullptr) { + std::optional<Token> Next = utils::lexer::findNextTokenSkippingComments( + ECD->getLocation(), *Result.SourceManager, getLangOpts()); + if (!Next.has_value() || Next->getLocation().isMacroID()) + continue; + llvm::SmallString<8> Str{" = "}; + ECD->getInitVal().toString(Str); + Diag << FixItHint::CreateInsertion(Next->getLocation(), Str); + } return; - DiagnosticBuilder Diag = - diag(Loc, "inital values in enum %0 are not consistent, consider " - "explicit initialization first, all or none of enumerators") - << Enum; - for (const EnumConstantDecl *ECD : Enum->enumerators()) - if (ECD->getInitExpr() == nullptr) { - SourceLocation ECDLoc = ECD->getEndLoc(); - if (ECDLoc.isInvalid() || ECDLoc.isMacroID()) - continue; - std::optional<Token> Next = utils::lexer::findNextTokenSkippingComments( - ECDLoc, *Result.SourceManager, getLangOpts()); - if (!Next.has_value() || Next->getLocation().isMacroID()) - continue; - llvm::SmallString<8> Str{" = "}; - ECD->getInitVal().toString(Str); - Diag << FixItHint::CreateInsertion(Next->getLocation(), Str); - } + } + if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("zero_first")) { + const EnumConstantDecl *ECD = *Enum->enumerators().begin(); + SourceLocation Loc = ECD->getLocation(); + if (Loc.isInvalid() || Loc.isMacroID()) + return; + DiagnosticBuilder Diag = + diag(Loc, "zero fist initial value in %0 can be ignored") << Enum; + cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts()); + return; + } + if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("linear")) { + SourceLocation Loc = Enum->getBeginLoc(); + if (Loc.isInvalid() || Loc.isMacroID()) + return; + DiagnosticBuilder Diag = + diag(Loc, "linear initial value in %0 can be ignored") << Enum; + for (const EnumConstantDecl *ECD : llvm::drop_begin(Enum->enumerators())) + cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts()); + return; + } } } // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h index ac1ac275df8b8c..7b2e280029ff37 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h @@ -20,13 +20,17 @@ namespace clang::tidy::readability { /// http://clang.llvm.org/extra/clang-tidy/checks/readability/enum-initial-value.html class EnumInitialValueCheck : public ClangTidyCheck { public: - EnumInitialValueCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + EnumInitialValueCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; std::optional<TraversalKind> getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } + +private: + const bool AllowExplicitZeroFirstInitialValue; + const bool AllowExplicitLinearInitialValues; }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst index 1c16a365255cba..fc9645088e4664 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst @@ -43,3 +43,33 @@ In an enumeration, the following three cases are accepted. e1, e2 = 2, }; + +Options +------- + +.. option:: AllowExplicitZeroFirstInitialValue + + If set to `false`, explicit initialized first enumerator is not allowed. + See examples below. Default is `true`. + + .. code-block:: c++ + + enum A { + e0 = 0, // not allowed if AllowExplicitZeroFirstInitialValue is false + e1, + e2, + }; + + +.. option:: AllowExplicitLinearInitialValues + + If set to `false`, linear initializations are not allowed. + See examples below. Default is `true`. + + .. code-block:: c++ + + enum A { + e0 = 1, // not allowed if AllowExplicitLinearInitialValues is false + e1 = 2, + e2 = 3, + }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c index 4b87183e480603..587f96dd282015 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c @@ -1,7 +1,13 @@ // RUN: %check_clang_tidy %s readability-enum-initial-value %t +// RUN: %check_clang_tidy -check-suffix=ENABLE %s readability-enum-initial-value %t -- \ +// RUN: -config='{CheckOptions: { \ +// RUN: readability-enum-initial-value.AllowExplicitZeroFirstInitialValue: false, \ +// RUN: readability-enum-initial-value.AllowExplicitLinearInitialValues: false, \ +// RUN: }}' enum EError { // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EError' are not consistent + // CHECK-MESSAGES-ENABLE: :[[@LINE-2]]:1: warning: inital values in enum 'EError' are not consistent EError_a = 1, EError_b, // CHECK-FIXES: EError_b = 2, @@ -23,14 +29,16 @@ enum EFirst { enum EAll { EAll_a = 1, EAll_b = 2, - EAll_c = 3, + EAll_c = 4, }; #define ENUMERATOR_1 EMacro1_b enum EMacro1 { // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EMacro1' are not consistent + // CHECK-MESSAGES-ENABLE: :[[@LINE-2]]:1: warning: inital values in enum 'EMacro1' are not consistent EMacro1_a = 1, ENUMERATOR_1, + // CHECK-FIXES: ENUMERATOR_1 = 2, EMacro1_c = 3, }; @@ -38,7 +46,35 @@ enum EMacro1 { #define ENUMERATOR_2 EMacro2_b = 2 enum EMacro2 { // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: inital values in enum 'EMacro2' are not consistent + // CHECK-MESSAGES-ENABLE: :[[@LINE-2]]:1: warning: inital values in enum 'EMacro2' are not consistent EMacro2_a = 1, ENUMERATOR_2, EMacro2_c, + // CHECK-FIXES: EMacro2_c = 3, +}; + +enum EnumZeroFirstInitialValue { + EnumZeroFirstInitialValue_0 = 0, + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero fist initial value in 'EnumZeroFirstInitialValue' can be ignored + // CHECK-FIXES-ENABLE: EnumZeroFirstInitialValue_0 , + EnumZeroFirstInitialValue_1, + EnumZeroFirstInitialValue_2, +}; + +enum EnumZeroFirstInitialValueWithComment { + EnumZeroFirstInitialValueWithComment_0 = /* == */ 0, + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero fist initial value in 'EnumZeroFirstInitialValueWithComment' can be ignored + // CHECK-FIXES-ENABLE: EnumZeroFirstInitialValueWithComment_0 /* == */ , + EnumZeroFirstInitialValueWithComment_1, + EnumZeroFirstInitialValueWithComment_2, +}; + +enum EnumLinearInitialValue { + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:1: warning: linear initial value in 'EnumLinearInitialValue' can be ignored + EnumLinearInitialValue_0 = 2, + // CHECK-FIXES-ENABLE: EnumLinearInitialValue_0 = 2, + EnumLinearInitialValue_1 = 3, + // CHECK-FIXES-ENABLE: EnumLinearInitialValue_1 , + EnumLinearInitialValue_2 = 4, + // CHECK-FIXES-ENABLE: EnumLinearInitialValue_2 , }; >From 038220713bfd3afe44eb07c6ddb416f0f39ab188 Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Thu, 28 Mar 2024 00:16:14 +0800 Subject: [PATCH 6/7] fix according to comment --- .../readability/EnumInitialValueCheck.cpp | 160 +++++++++--------- .../readability/EnumInitialValueCheck.h | 6 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 +- .../checks/readability/enum-initial-value.rst | 12 +- .../checkers/readability/enum-initial-value.c | 22 +-- 5 files changed, 105 insertions(+), 99 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp index 119e59cd721a14..5a3a439e8ae058 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -20,44 +20,24 @@ using namespace clang::ast_matchers; namespace clang::tidy::readability { -EnumInitialValueCheck::EnumInitialValueCheck(StringRef Name, - ClangTidyContext *Context) - : ClangTidyCheck(Name, Context), - AllowExplicitZeroFirstInitialValue( - Options.get("AllowExplicitZeroFirstInitialValue", true)), - AllowExplicitLinearInitialValues( - Options.get("AllowExplicitLinearInitialValues", true)) {} - -void EnumInitialValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "AllowExplicitZeroFirstInitialValue", - AllowExplicitZeroFirstInitialValue); - Options.store(Opts, "AllowExplicitLinearInitialValues", - AllowExplicitLinearInitialValues); -} - -namespace { - -bool isNoneEnumeratorsInitialized(const EnumDecl &Node) { +static bool isNoneEnumeratorsInitialized(const EnumDecl &Node) { return llvm::all_of(Node.enumerators(), [](const EnumConstantDecl *ECD) { return ECD->getInitExpr() == nullptr; }); } -bool isOnlyFirstEnumeratorsInitialized(const EnumDecl &Node) { +static bool isOnlyFirstEnumeratorInitialized(const EnumDecl &Node) { bool IsFirst = true; - for (const EnumConstantDecl *ECD : Node.enumerators()) - if (IsFirst) { - IsFirst = false; - if (ECD->getInitExpr() == nullptr) - return false; - } else { - if (ECD->getInitExpr() != nullptr) - return false; - } + for (const EnumConstantDecl *ECD : Node.enumerators()) { + if ((IsFirst && ECD->getInitExpr() == nullptr) || + (!IsFirst && ECD->getInitExpr() != nullptr)) + return false; + IsFirst = false; + } return !IsFirst; } -bool isAllEnumeratorsInitialized(const EnumDecl &Node) { +static bool areAllEnumeratorsInitialized(const EnumDecl &Node) { return llvm::all_of(Node.enumerators(), [](const EnumConstantDecl *ECD) { return ECD->getInitExpr() != nullptr; }); @@ -65,24 +45,52 @@ bool isAllEnumeratorsInitialized(const EnumDecl &Node) { /// Check if \p Enumerator is initialized with a (potentially negated) \c /// IntegerLiteral. -bool isInitializedByLiteral(const EnumConstantDecl *Enumerator) { +static bool isInitializedByLiteral(const EnumConstantDecl *Enumerator) { const Expr *const Init = Enumerator->getInitExpr(); if (!Init) return false; return Init->isIntegerConstantExpr(Enumerator->getASTContext()); } +static void cleanInitialValue(DiagnosticBuilder &Diag, + const EnumConstantDecl *ECD, + const SourceManager &SM, + const LangOptions &LangOpts) { + std::optional<Token> EqualToken = utils::lexer::findNextTokenSkippingComments( + ECD->getLocation(), SM, LangOpts); + if (!EqualToken.has_value()) + return; + SourceLocation EqualLoc{EqualToken->getLocation()}; + if (EqualLoc.isInvalid() || EqualLoc.isMacroID()) + return; + SourceRange InitExprRange = ECD->getInitExpr()->getSourceRange(); + if (InitExprRange.isInvalid() || InitExprRange.getBegin().isMacroID() || + InitExprRange.getEnd().isMacroID()) + return; + Diag << FixItHint::CreateRemoval(EqualLoc) + << FixItHint::CreateRemoval(InitExprRange); + return; +} + +namespace { + +AST_MATCHER(EnumDecl, isMacro) { + SourceLocation Loc = Node.getBeginLoc(); + return Loc.isMacroID(); +} + AST_MATCHER(EnumDecl, hasConsistentInitialValues) { return isNoneEnumeratorsInitialized(Node) || - isOnlyFirstEnumeratorsInitialized(Node) || - isAllEnumeratorsInitialized(Node); + isOnlyFirstEnumeratorInitialized(Node) || + areAllEnumeratorsInitialized(Node); } -AST_MATCHER(EnumDecl, hasZeroFirstInitialValue) { - if (Node.enumerators().empty()) +AST_MATCHER(EnumDecl, hasZeroInitialValueForFirstEnumerator) { + EnumDecl::enumerator_range Enumerators = Node.enumerators(); + if (Enumerators.empty()) return false; - const EnumConstantDecl *ECD = *Node.enumerators().begin(); - return isOnlyFirstEnumeratorsInitialized(Node) && + const EnumConstantDecl *ECD = *Enumerators.begin(); + return isOnlyFirstEnumeratorInitialized(Node) && isInitializedByLiteral(ECD) && ECD->getInitVal().isZero(); } @@ -93,16 +101,16 @@ AST_MATCHER(EnumDecl, hasZeroFirstInitialValue) { /// bitmask, evident when enumerators are only initialized with (potentially /// negative) integer literals, are ignored. This is also the case when all /// enumerators are powers of two (e.g., 0, 1, 2). -AST_MATCHER(EnumDecl, hasLinearInitialValues) { - if (Node.enumerators().empty()) +AST_MATCHER(EnumDecl, hasSequentialInitialValues) { + EnumDecl::enumerator_range Enumerators = Node.enumerators(); + if (Enumerators.empty()) return false; const EnumConstantDecl *const FirstEnumerator = *Node.enumerator_begin(); llvm::APSInt PrevValue = FirstEnumerator->getInitVal(); if (!isInitializedByLiteral(FirstEnumerator)) return false; bool AllEnumeratorsArePowersOfTwo = true; - for (const EnumConstantDecl *Enumerator : - llvm::drop_begin(Node.enumerators())) { + for (const EnumConstantDecl *Enumerator : llvm::drop_begin(Enumerators)) { const llvm::APSInt NewValue = Enumerator->getInitVal(); if (NewValue != ++PrevValue) return false; @@ -116,45 +124,42 @@ AST_MATCHER(EnumDecl, hasLinearInitialValues) { } // namespace +EnumInitialValueCheck::EnumInitialValueCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowExplicitZeroFirstInitialValue( + Options.get("AllowExplicitZeroFirstInitialValue", true)), + AllowExplicitSequentialInitialValues( + Options.get("AllowExplicitSequentialInitialValues", true)) {} + +void EnumInitialValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowExplicitZeroFirstInitialValue", + AllowExplicitZeroFirstInitialValue); + Options.store(Opts, "AllowExplicitSequentialInitialValues", + AllowExplicitSequentialInitialValues); +} + void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - enumDecl(unless(hasConsistentInitialValues())).bind("inconsistent"), + enumDecl(unless(isMacro()), unless(hasConsistentInitialValues())) + .bind("inconsistent"), this); if (!AllowExplicitZeroFirstInitialValue) - Finder->addMatcher(enumDecl(hasZeroFirstInitialValue()).bind("zero_first"), + Finder->addMatcher( + enumDecl(hasZeroInitialValueForFirstEnumerator()).bind("zero_first"), + this); + if (!AllowExplicitSequentialInitialValues) + Finder->addMatcher(enumDecl(unless(isMacro()), hasSequentialInitialValues()) + .bind("sequential"), this); - if (!AllowExplicitLinearInitialValues) - Finder->addMatcher(enumDecl(hasLinearInitialValues()).bind("linear"), this); -} - -static void cleanInitialValue(DiagnosticBuilder &Diag, - const EnumConstantDecl *ECD, - const SourceManager &SM, - const LangOptions &LangOpts) { - std::optional<Token> EqualToken = utils::lexer::findNextTokenSkippingComments( - ECD->getLocation(), SM, LangOpts); - if (!EqualToken.has_value()) - return; - SourceLocation EqualLoc{EqualToken->getLocation()}; - if (EqualLoc.isInvalid() || EqualLoc.isMacroID()) - return; - SourceRange InitExprRange = ECD->getInitExpr()->getSourceRange(); - if (InitExprRange.isInvalid() || InitExprRange.getBegin().isMacroID() || - InitExprRange.getEnd().isMacroID()) - return; - Diag << FixItHint::CreateRemoval(EqualLoc) - << FixItHint::CreateRemoval(InitExprRange); - return; } void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("inconsistent")) { - SourceLocation Loc = Enum->getBeginLoc(); - if (Loc.isInvalid() || Loc.isMacroID()) - return; DiagnosticBuilder Diag = - diag(Loc, "inital values in enum %0 are not consistent, consider " - "explicit initialization first, all or none of enumerators") + diag(Enum->getBeginLoc(), + "inital values in enum %0 are not consistent, consider " + "explicit initialization first, all or none of enumerators") << Enum; for (const EnumConstantDecl *ECD : Enum->enumerators()) if (ECD->getInitExpr() == nullptr) { @@ -168,22 +173,23 @@ void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { } return; } + if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("zero_first")) { - const EnumConstantDecl *ECD = *Enum->enumerators().begin(); + const EnumConstantDecl *ECD = *Enum->enumerator_begin(); SourceLocation Loc = ECD->getLocation(); if (Loc.isInvalid() || Loc.isMacroID()) return; - DiagnosticBuilder Diag = - diag(Loc, "zero fist initial value in %0 can be ignored") << Enum; + DiagnosticBuilder Diag = diag(Loc, "zero initial value for the first " + "enumerator in %0 can be disregarded") + << Enum; cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts()); return; } - if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("linear")) { - SourceLocation Loc = Enum->getBeginLoc(); - if (Loc.isInvalid() || Loc.isMacroID()) - return; + if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("sequential")) { DiagnosticBuilder Diag = - diag(Loc, "linear initial value in %0 can be ignored") << Enum; + diag(Enum->getBeginLoc(), + "sequential initial value in %0 can be ignored") + << Enum; for (const EnumConstantDecl *ECD : llvm::drop_begin(Enum->enumerators())) cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts()); return; diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h index 7b2e280029ff37..66087e4ee170da 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h @@ -13,8 +13,8 @@ namespace clang::tidy::readability { -/// Detects explicit initialization of a part of enumerators in an enumeration, -/// and relying on compiler to initialize the others. +/// Enforces consistent style for enumerators' initialization, covering three +/// styles: none, first only, or all initialized explicitly. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/readability/enum-initial-value.html @@ -30,7 +30,7 @@ class EnumInitialValueCheck : public ClangTidyCheck { private: const bool AllowExplicitZeroFirstInitialValue; - const bool AllowExplicitLinearInitialValues; + const bool AllowExplicitSequentialInitialValues; }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 50bceec2f604d4..c85b4d06135f8c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -126,8 +126,8 @@ New checks - New :doc:`readability-enum-initial-value <clang-tidy/checks/readability/enum-initial-value>` check. - Detects explicit initialization of a part of enumerators in an enumeration, and - relying on compiler to initialize the others. + Enforces consistent style for enumerators' initialization, covering three + styles: none, first only, or all initialized explicitly. - New :doc:`readability-use-std-min-max <clang-tidy/checks/readability/use-std-min-max>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst index fc9645088e4664..660efc1eaff3e5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst @@ -3,8 +3,8 @@ readability-enum-initial-value ============================== -Detects explicit initialization of a part of enumerators in an enumeration, and -relying on compiler to initialize the others. +Enforces consistent style for enumerators' initialization, covering three +styles: none, first only, or all initialized explicitly. When adding new enumerations, inconsistent initial value will cause potential enumeration value conflicts. @@ -49,7 +49,7 @@ Options .. option:: AllowExplicitZeroFirstInitialValue - If set to `false`, explicit initialized first enumerator is not allowed. + If set to `false`, the first enumerator must not be explicitly initialized. See examples below. Default is `true`. .. code-block:: c++ @@ -61,15 +61,15 @@ Options }; -.. option:: AllowExplicitLinearInitialValues +.. option:: AllowExplicitSequentialInitialValues - If set to `false`, linear initializations are not allowed. + If set to `false`, sequential initializations are not allowed. See examples below. Default is `true`. .. code-block:: c++ enum A { - e0 = 1, // not allowed if AllowExplicitLinearInitialValues is false + e0 = 1, // not allowed if AllowExplicitSequentialInitialValues is false e1 = 2, e2 = 3, }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c index 587f96dd282015..c66288cbe3e957 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c @@ -2,7 +2,7 @@ // RUN: %check_clang_tidy -check-suffix=ENABLE %s readability-enum-initial-value %t -- \ // RUN: -config='{CheckOptions: { \ // RUN: readability-enum-initial-value.AllowExplicitZeroFirstInitialValue: false, \ -// RUN: readability-enum-initial-value.AllowExplicitLinearInitialValues: false, \ +// RUN: readability-enum-initial-value.AllowExplicitSequentialInitialValues: false, \ // RUN: }}' enum EError { @@ -55,7 +55,7 @@ enum EMacro2 { enum EnumZeroFirstInitialValue { EnumZeroFirstInitialValue_0 = 0, - // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero fist initial value in 'EnumZeroFirstInitialValue' can be ignored + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero initial value for the first enumerator in 'EnumZeroFirstInitialValue' can be disregarded // CHECK-FIXES-ENABLE: EnumZeroFirstInitialValue_0 , EnumZeroFirstInitialValue_1, EnumZeroFirstInitialValue_2, @@ -63,18 +63,18 @@ enum EnumZeroFirstInitialValue { enum EnumZeroFirstInitialValueWithComment { EnumZeroFirstInitialValueWithComment_0 = /* == */ 0, - // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero fist initial value in 'EnumZeroFirstInitialValueWithComment' can be ignored + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero initial value for the first enumerator in 'EnumZeroFirstInitialValueWithComment' can be disregarded // CHECK-FIXES-ENABLE: EnumZeroFirstInitialValueWithComment_0 /* == */ , EnumZeroFirstInitialValueWithComment_1, EnumZeroFirstInitialValueWithComment_2, }; -enum EnumLinearInitialValue { - // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:1: warning: linear initial value in 'EnumLinearInitialValue' can be ignored - EnumLinearInitialValue_0 = 2, - // CHECK-FIXES-ENABLE: EnumLinearInitialValue_0 = 2, - EnumLinearInitialValue_1 = 3, - // CHECK-FIXES-ENABLE: EnumLinearInitialValue_1 , - EnumLinearInitialValue_2 = 4, - // CHECK-FIXES-ENABLE: EnumLinearInitialValue_2 , +enum EnumSequentialInitialValue { + // CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:1: warning: sequential initial value in 'EnumSequentialInitialValue' can be ignored + EnumSequentialInitialValue_0 = 2, + // CHECK-FIXES-ENABLE: EnumSequentialInitialValue_0 = 2, + EnumSequentialInitialValue_1 = 3, + // CHECK-FIXES-ENABLE: EnumSequentialInitialValue_1 , + EnumSequentialInitialValue_2 = 4, + // CHECK-FIXES-ENABLE: EnumSequentialInitialValue_2 , }; >From 36549193b5fe0d64a8e30c3266a9eaed66c55b6e Mon Sep 17 00:00:00 2001 From: Congcong Cai <congcongcai0...@163.com> Date: Sun, 31 Mar 2024 08:50:30 +0800 Subject: [PATCH 7/7] fix comments --- .../readability/EnumInitialValueCheck.cpp | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp index 5a3a439e8ae058..f1bd5bd17b8a49 100644 --- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp @@ -56,17 +56,18 @@ static void cleanInitialValue(DiagnosticBuilder &Diag, const EnumConstantDecl *ECD, const SourceManager &SM, const LangOptions &LangOpts) { + const SourceRange InitExprRange = ECD->getInitExpr()->getSourceRange(); + if (InitExprRange.isInvalid() || InitExprRange.getBegin().isMacroID() || + InitExprRange.getEnd().isMacroID()) + return; std::optional<Token> EqualToken = utils::lexer::findNextTokenSkippingComments( ECD->getLocation(), SM, LangOpts); - if (!EqualToken.has_value()) + if (!EqualToken.has_value() || + EqualToken.value().getKind() != tok::TokenKind::equal) return; - SourceLocation EqualLoc{EqualToken->getLocation()}; + const SourceLocation EqualLoc{EqualToken->getLocation()}; if (EqualLoc.isInvalid() || EqualLoc.isMacroID()) return; - SourceRange InitExprRange = ECD->getInitExpr()->getSourceRange(); - if (InitExprRange.isInvalid() || InitExprRange.getBegin().isMacroID() || - InitExprRange.getEnd().isMacroID()) - return; Diag << FixItHint::CreateRemoval(EqualLoc) << FixItHint::CreateRemoval(InitExprRange); return; @@ -86,7 +87,7 @@ AST_MATCHER(EnumDecl, hasConsistentInitialValues) { } AST_MATCHER(EnumDecl, hasZeroInitialValueForFirstEnumerator) { - EnumDecl::enumerator_range Enumerators = Node.enumerators(); + const EnumDecl::enumerator_range Enumerators = Node.enumerators(); if (Enumerators.empty()) return false; const EnumConstantDecl *ECD = *Enumerators.begin(); @@ -102,7 +103,7 @@ AST_MATCHER(EnumDecl, hasZeroInitialValueForFirstEnumerator) { /// negative) integer literals, are ignored. This is also the case when all /// enumerators are powers of two (e.g., 0, 1, 2). AST_MATCHER(EnumDecl, hasSequentialInitialValues) { - EnumDecl::enumerator_range Enumerators = Node.enumerators(); + const EnumDecl::enumerator_range Enumerators = Node.enumerators(); if (Enumerators.empty()) return false; const EnumConstantDecl *const FirstEnumerator = *Node.enumerator_begin(); @@ -159,24 +160,24 @@ void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) { DiagnosticBuilder Diag = diag(Enum->getBeginLoc(), "inital values in enum %0 are not consistent, consider " - "explicit initialization first, all or none of enumerators") + "explicit initialization all, none or only the first enumerators") << Enum; for (const EnumConstantDecl *ECD : Enum->enumerators()) if (ECD->getInitExpr() == nullptr) { - std::optional<Token> Next = utils::lexer::findNextTokenSkippingComments( - ECD->getLocation(), *Result.SourceManager, getLangOpts()); - if (!Next.has_value() || Next->getLocation().isMacroID()) + const SourceLocation EndLoc = Lexer::getLocForEndOfToken( + ECD->getLocation(), 0, *Result.SourceManager, getLangOpts()); + if (EndLoc.isMacroID()) continue; llvm::SmallString<8> Str{" = "}; ECD->getInitVal().toString(Str); - Diag << FixItHint::CreateInsertion(Next->getLocation(), Str); + Diag << FixItHint::CreateInsertion(EndLoc, Str); } return; } if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("zero_first")) { const EnumConstantDecl *ECD = *Enum->enumerator_begin(); - SourceLocation Loc = ECD->getLocation(); + const SourceLocation Loc = ECD->getLocation(); if (Loc.isInvalid() || Loc.isMacroID()) return; DiagnosticBuilder Diag = diag(Loc, "zero initial value for the first " _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits