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