Add check 'modernize-use-enum-class' #138282
Open
+161
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Warn on non-class enum definitions as suggested by the Core Guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Philipp Jung (JungPhilipp) ChangesWarn on non-class enum definitions as suggested by the Core Guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class Full diff: https://github.com/llvm/llvm-project/pull/138282.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index 4f68c487cac9d..92e6866c5f17b 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -34,6 +34,7 @@ add_clang_library(clangTidyModernizeModule
UseDefaultMemberInitCheck.cpp
UseDesignatedInitializersCheck.cpp
UseEmplaceCheck.cpp
+ UseEnumClassCheck.cpp
UseEqualsDefaultCheck.cpp
UseEqualsDeleteCheck.cpp
UseNodiscardCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index 1860759332063..30e6938dbd7db 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -35,6 +35,7 @@
#include "UseDefaultMemberInitCheck.h"
#include "UseDesignatedInitializersCheck.h"
#include "UseEmplaceCheck.h"
+#include "UseEnumClassCheck.h"
#include "UseEqualsDefaultCheck.h"
#include "UseEqualsDeleteCheck.h"
#include "UseNodiscardCheck.h"
@@ -107,6 +108,7 @@ class ModernizeModule : public ClangTidyModule {
CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
"modernize-use-default-member-init");
CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
+ CheckFactories.registerCheck<UseEnumClassCheck>("modernize-use-enum-class");
CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
CheckFactories.registerCheck<UseEqualsDeleteCheck>(
"modernize-use-equals-delete");
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.cpp
new file mode 100644
index 0000000000000..9fc3614aaf498
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.cpp
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.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 "UseEnumClassCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::modernize {
+
+void UseEnumClassCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ traverse(TK_AsIs,
+ enumDecl(unless(isScoped()), unless(hasParent(recordDecl()))))
+ .bind("unscoped_enum"),
+ this);
+}
+
+void UseEnumClassCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *UnscopedEnum = Result.Nodes.getNodeAs<EnumDecl>("unscoped_enum");
+
+ diag(UnscopedEnum->getLocation(),
+ "enum %0 is unscoped, use enum class instead")
+ << UnscopedEnum;
+ diag(UnscopedEnum->getLocation(), "insert 'class'", DiagnosticIDs::Note)
+ << FixItHint::CreateInsertion(UnscopedEnum->getLocation(), "class ");
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.h
new file mode 100644
index 0000000000000..9cfb2024b9cfd
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseEnumClassCheck.h
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEENUMCLASSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEENUMCLASSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::modernize {
+
+/// Check for unscoped enums that are not contained in classes/structs.
+/// Suggest to use scoped enums (enum class) instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-enum-class.html
+class UseEnumClassCheck : public ClangTidyCheck {
+public:
+ UseEnumClassCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_ENUM_CLASS_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b4792d749a86c..f39dcecae63f2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -115,6 +115,11 @@ New checks
Gives warnings for tagged unions, where the number of tags is
different from the number of data members inside the union.
+- New :doc:`modernize-use-enum-class
+ <clang-tidy/checks/modernize/use-enum-class>` check.
+
+ Finds plain non-class enum definitions that could use ``enum class``.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e3dfabba8fad1..6533828a96fdf 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -297,6 +297,7 @@ Clang-Tidy Checks
:doc:`modernize-use-default-member-init <modernize/use-default-member-init>`, "Yes"
:doc:`modernize-use-designated-initializers <modernize/use-designated-initializers>`, "Yes"
:doc:`modernize-use-emplace <modernize/use-emplace>`, "Yes"
+ :doc:`modernize-use-enum-class <modernize/use-enum-class>`, "Yes"
:doc:`modernize-use-equals-default <modernize/use-equals-default>`, "Yes"
:doc:`modernize-use-equals-delete <modernize/use-equals-delete>`, "Yes"
:doc:`modernize-use-nodiscard <modernize/use-nodiscard>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst
new file mode 100644
index 0000000000000..3adb6e204ad92
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - modernize-use-enum-class
+
+modernize-use-enum-class
+=============================
+
+Scoped enums (enum class) should be preferred over unscoped enums:
+https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class
+
+Unscoped enums in classes are not reported since it is a well
+established pattern to limit the scope of plain enums.
+
+Example:
+
+.. code-block:: c++
+
+ enum E {}; // use "enum class E {};" instead
+ enum class E {}; // OK
+
+ struct S {
+ enum E {}; // OK, scope already limited
+ };
+
+ namespace N {
+ enum E {}; // use "enum class E {};" instead
+ // report since it is hard to detect how large the surrounding namespace is
+ }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-enum-class.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-enum-class.cpp
new file mode 100644
index 0000000000000..9712fbc0aac4a
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-enum-class.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-use-enum-class %t
+
+enum E {};
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+
+enum class EC {};
+
+struct S {
+ enum E {};
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+ // Ignore unscoped enums in recordDecl
+ enum class EC {};
+};
+
+class C {
+ enum E {};
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+ // Ignore unscoped enums in recordDecl
+ enum class EC {};
+};
+
+template<class T>
+class TC {
+ enum E {};
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+ // Ignore unscoped enums in recordDecl
+ enum class EC {};
+};
+
+union U {
+ enum E {};
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+ // Ignore unscoped enums in recordDecl
+ enum class EC {};
+};
+
+namespace {
+enum E {};
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+enum class EC {};
+} // namespace
+
+namespace N {
+enum E {};
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'E' is unscoped, use enum class instead [modernize-use-enum-class]
+enum class EC {};
+} // namespace N
+
+template<enum ::EC>
+static void foo();
+
+using enum S::E;
+using enum S::EC;
+
+enum ForwardE : int;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'ForwardE' is unscoped, use enum class instead [modernize-use-enum-class]
+
+enum class ForwardEC : int;
|
vbvictor
reviewed
May 2, 2025
There was a problem hiding this comment.
Overall, good niche check that may find its user. However, some work need to be done.
Move this check to cppcoreguidelines- section, you can use clang-tools-extra\clang-tidy\rename_check.py for that.
modernize- checks are used for smooth codebase improvements, and they should not (hopefully) break any existing code in any way (e.g. no changes to behavior, no new compiler errors). In your case, automatically converting enum to enum class will probably create hundreds compiler errors for big codebases, and that does not align with modernize philosophy.
|
|
||
| void UseEnumClassCheck::registerMatchers(MatchFinder *Finder) { | ||
| Finder->addMatcher( | ||
| traverse(TK_AsIs, |
There was a problem hiding this comment.
I think we don't need to traverse AsIs in this check, UnlessSpelledInSource must do the job.
You should remove this traverse call and override getCheckTraversalKind() method of ClangTidyCheck.
See for reference:
| void UseEnumClassCheck::registerMatchers(MatchFinder *Finder) { | ||
| Finder->addMatcher( | ||
| traverse(TK_AsIs, | ||
| enumDecl(unless(isScoped()), unless(hasParent(recordDecl())))) |
There was a problem hiding this comment.
Suggested change
| enumDecl(unless(isScoped()), unless(hasParent(recordDecl())))) | |
| enumDecl(unless(isScoped()) |
If we strictly follow CppCoreGuidelines (which we should do it cppcoreguidelines- checks), there are no mention of "established pattern to limit the scope".
So I think we need to remove this condition or at least create an option like IgnoreInClasses to optionally relax this check.
Comment on lines
+30
to
+31
| diag(UnscopedEnum->getLocation(), "insert 'class'", DiagnosticIDs::Note) | ||
| << FixItHint::CreateInsertion(UnscopedEnum->getLocation(), "class "); |
There was a problem hiding this comment.
I think we should not create fix-its that will break code in many places (possibly hundreds).
Suggested change
| diag(UnscopedEnum->getLocation(), "insert 'class'", DiagnosticIDs::Note) | |
| << FixItHint::CreateInsertion(UnscopedEnum->getLocation(), "class "); |
| const auto *UnscopedEnum = Result.Nodes.getNodeAs<EnumDecl>("unscoped_enum"); | ||
|
|
||
| diag(UnscopedEnum->getLocation(), | ||
| "enum %0 is unscoped, use enum class instead") |
There was a problem hiding this comment.
I think it is better readable this way
Suggested change
| "enum %0 is unscoped, use enum class instead") | |
| "enum %0 is unscoped, use 'enum class' instead") |
| void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
| void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||
| bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { | ||
| return LangOpts.CPlusPlus; |
There was a problem hiding this comment.
Suggested change
| return LangOpts.CPlusPlus; | |
| return LangOpts.CPlusPlus11; |
| - New :doc:`modernize-use-enum-class | ||
| <clang-tidy/checks/modernize/use-enum-class>` check. | ||
|
|
||
| Finds plain non-class enum definitions that could use ``enum class``. |
There was a problem hiding this comment.
Suggested change
| Finds plain non-class enum definitions that could use ``enum class``. | |
| Finds plain non-class ``enum`` definitions that could use ``enum class``. |
Comment on lines
+9
to
+10
| Unscoped enums in classes are not reported since it is a well | ||
| established pattern to limit the scope of plain enums. |
There was a problem hiding this comment.
This should be edited into an option or removed completely, as in #138282 (comment).
| @@ -0,0 +1,58 @@ | |||
| // RUN: %check_clang_tidy -std=c++17-or-later %s modernize-use-enum-class %t | |||
There was a problem hiding this comment.
Suggested change
| // RUN: %check_clang_tidy -std=c++17-or-later %s modernize-use-enum-class %t | |
| // RUN: %check_clang_tidy -std=c++11-or-later %s modernize-use-enum-class %t |
| modernize-use-enum-class | ||
| ============================= | ||
|
|
||
| Scoped enums (enum class) should be preferred over unscoped enums: |
There was a problem hiding this comment.
The first statement in this doc should be the same as in ReleaseNotes.rst.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warn on non-class enum definitions as suggested by the Core Guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class