-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add check 'cppcoreguidelines-use-enum-class' #138282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add check 'cppcoreguidelines-use-enum-class' #138282
Conversation
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;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/modernize/use-enum-class.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-enum-class.rst
Outdated
Show resolved
Hide resolved
Your branch seems to be way behind current |
Warn on non-class enum definitions as suggested by the Core Guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class
9e2d442
to
f7463f6
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits and documentation suggestions/improvements.
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-enum-class.rst
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-enum-class.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-enum-class.rst
Outdated
Show resolved
Hide resolved
...st/clang-tidy/checkers/cppcoreguidelines/use-enum-class-ignore-unscoped-enums-in-classes.cpp
Outdated
Show resolved
Hide resolved
...st/clang-tidy/checkers/cppcoreguidelines/use-enum-class-ignore-unscoped-enums-in-classes.cpp
Show resolved
Hide resolved
...st/clang-tidy/checkers/cppcoreguidelines/use-enum-class-ignore-unscoped-enums-in-classes.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-enum-class.rst
Outdated
Show resolved
Hide resolved
Thanks to everybody for the quick and thorough reviews. I really appreciate the help! |
You should wait for an approval from one of the clang-tidy maintainers (PiotrZSL, carlosgalvezp, HerrCai0907) that will also help merge this PR. It can take some time, but the new release will only be in July, so don't need to worry about missing your check for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, few nits with docs.
Ping, @HerrCai0907, @carlosgalvezp, @PiotrZSL if you wish to leave a review.
I suppose in 1-2 weeks I'll land the PR.
|
||
namespace clang::tidy::cppcoreguidelines { | ||
|
||
/// Check for unscoped enums and suggest to use scoped enums (enum class). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sync this with sentences in docs and ReleaseNotes
namespace clang::tidy::cppcoreguidelines { | ||
|
||
/// Check for unscoped enums and suggest to use scoped enums (enum class). | ||
/// Optionally, ignore unscoped enums in classes via IgnoreUnscopedEnumsInClasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, only the sentence in ReleaseNotes should be placed here
Finds unscoped (non-class) ``enum`` declarations and suggests using | ||
``enum class`` instead. | ||
|
||
This check implements `Enum.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be at the end of documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it?
I've seen that most of cppcoreguidelines checks have this in the beginning.
I suppose it's encouraging user to read actual guideline before reading whole check documentation.
Some random examples I clicked right now:
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.html
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-do-while.html
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/interfaces-global-init.html
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index.html
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through more examples, I see that most of cppcoreguidelines-pro-type
checks and a couple of others have link in the end, but others tend to have up front.
I genuinely prefer it up front because it's helpful to go to core-guideline after reading a brief description of the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be good idea to make link location consistent across all checks documentation.
|
||
struct S { | ||
enum E {}; | ||
// CHECK-MESSAGES-NOT: :[[@LINE-1]]:8: warning: enum 'E' is unscoped, use 'enum class' instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not use CHECK-MESSAGES-NOT, please remove
@@ -0,0 +1,18 @@ | |||
// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-use-enum-class %t -- \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be possible to merge these two test files, and provide a specific CHECK-MESSAGES-FULL (or similar) that only warns when the option is not enabled.
|
||
enum ForwardE : int; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'ForwardE' is unscoped, use 'enum class' instead | ||
enum class ForwardEC : int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a test with enum struct
as well, Jason Turner shows that unconventional pattern a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some small comments :)
Also, please rebase (there are some merge conflicts), and also mark conversations as resolved (I see there's a few remaining) |
Warn on non-class enum definitions as suggested by the Core Guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class