[clang-tidy] add new check readability-enum-initial-value #86129

HerrCai0907 · 2024-03-21T15:01:32Z

Fixes: #85243.

Fixes: llvm#85243.

llvmbot · 2024-03-21T15:02:13Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes

Fixes: #85243.

Full diff: https://github.com/llvm/llvm-project/pull/86129.diff

9 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/CMakeLists.txt (+1)
(added) clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp (+82)
(added) clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h (+31)
(modified) clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp (+3)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
(added) clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst (+45)
(added) clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c (+27)
(added) clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp (+27)

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,
+};

github-actions · 2024-03-21T15:04:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

PiotrZSL

Few things to consider.
Overall looks +- fine.

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.h

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst

clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.cpp

…l-value

5chmidti

Just nits and a comment regarding the linear, all initialized enum. Also, all other checks use west-const from what I can tell.

clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

PiotrZSL

Overall looks fine.
Some minor style issues.

Try getting rid of findNextTokenSkippingComments, and maybe just jump to begin() + token width

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/enum-initial-value.rst

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

HerrCai0907 · 2024-03-27T16:19:15Z

Try getting rid of findNextTokenSkippingComments, and maybe just jump to begin() + token width

Do you think it is a good idea to use ECD->getLocation().getLocWithOffset(ECD->getName().size())?

HerrCai0907 · 2024-03-28T01:38:43Z

Try getting rid of findNextTokenSkippingComments, and maybe just jump to begin() + token width

Do you think it is a good idea to use ECD->getLocation().getLocWithOffset(ECD->getName().size())?

It doesn't work for macro. I think the most precise way is to fix it is using findNextTokenSkippingComments. Why should we avoid to use findNextTokenSkippingComments? Is it for performance reasons?

5chmidti

I only have some minor comments, otherwise looks good to me

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

PiotrZSL

For me looks fine (had to check how it behave when coma is in next line).
Some ideas for separate check or future options:

enforce that enums are sorted by value
enforce that enums are sorted by name
enforce that there is no duplicated values (bugprone), there is warning for implicit duplicate, but not explicit.

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp

[clang-tidy] add new check readability-enum-initial-value

4e0845a

Fixes: llvm#85243.

HerrCai0907 requested review from PiotrZSL and 5chmidti March 21, 2024 15:01

llvmbot added clang-tools-extra clang-tidy labels Mar 21, 2024

fix format

d149661

PiotrZSL reviewed Mar 21, 2024

View reviewed changes

HerrCai0907 added 2 commits March 22, 2024 09:22

Merge branch 'main' into new-clang-tidy-check/readability/enum-initia…

e2088f6

…l-value

fix msg

089815c

5chmidti reviewed Mar 23, 2024

View reviewed changes

HerrCai0907 added 2 commits March 26, 2024 22:00

fix

eaeb491

extend check

02fd17b

HerrCai0907 requested review from PiotrZSL and 5chmidti March 26, 2024 15:31

PiotrZSL requested changes Mar 26, 2024

View reviewed changes

fix according to comment

0382207

HerrCai0907 requested a review from PiotrZSL March 28, 2024 10:32

5chmidti reviewed Mar 30, 2024

View reviewed changes

HerrCai0907 added 2 commits March 31, 2024 08:50

fix comments

3654919

fix

617e15a

PiotrZSL approved these changes Mar 31, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp Show resolved Hide resolved

HerrCai0907 merged commit 3365d62 into llvm:main Apr 1, 2024

HerrCai0907 deleted the new-clang-tidy-check/readability/enum-initial-value branch April 1, 2024 07:54

[clang-tidy] add new check readability-enum-initial-value #86129

[clang-tidy] add new check readability-enum-initial-value #86129

Uh oh!

Conversation

HerrCai0907 commented Mar 21, 2024

Uh oh!

llvmbot commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

5chmidti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HerrCai0907 commented Mar 27, 2024

Uh oh!

HerrCai0907 commented Mar 28, 2024

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited

Loading

5chmidti left a comment •

edited

Loading

PiotrZSL left a comment •

edited

Loading