[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D85697#2234468 , 
@janosbenjaminantal wrote:

> The known "limitations" are mostly similar to other checks:
>
> - It cannot detect unfixable cases from other translation units. Practically 
> that means if an `enum` is used in multiple source files, one of them might 
> end up not fixed. I tried to work around this, but I haven't found any 
> solution for this, moreover this cause problems for other checks also. 
> Therefore I think it shouldn't be a blocking issue.

That's what the run_clang_tidy.py script is meant to handle.

> - It doesn't take into account if an `enum` is defined in a header that is 
> filtered out. Similarly to the previous one, I tried to find a solution for 
> this, but I was unable (the `ClangTidyCheck` class can access the header 
> filter information, but it doesn't expose it for the descendant classes). I 
> also checked other checks, and they behave in the same way. Therefore I also 
> think it is shouldn't be a blocking issue.

I recently pushed an upgrade to readability-identifier-naming where it would 
check the naming style for identifiers declared in header files, maybe thats 
something this could also use, this is the commit 4888c9ce97d8 


> It is not strongly connected to this review, but in the future I am planning 
> to extend the check with:
>
> - options to exclude enums, because changing them to scoped enumerations 
> might not be suitable for every cases

Not strictly necessary, if people don't want the fix they could annotate the 
code with a `// NOLINT(*prefer-unscoped-enums)` comment.

> - options to force the doable fixes: based on my little experience it might 
> be easier to force the doable fixes and manually fix the remaining ones

Forcing the fix is usually just a case of converting implicit cast usages of 
the constants into static casts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

It may be a pain, but I'd also like to see an option like `RemovePrefix`.
Generally most unscoped enums constants have prefixes based on the enum name. A 
simple way around this would just be to get the longest common prefix in all 
the enums constants. There is probably some other heuristics that could be used 
like maybe stop after you reach an underscore or change of case but probably 
not essential.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:82
+
+  Checks for unscoped enumerations.
+

Could this perhaps be a little more informative?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums.rst:35-36
+
+Note: Although ``enum struct`` and ``enum class`` are exactly equivalent, the
+latter is used mainly.

I'd prefer if this was user controlled, An option like `UseEnumStruct` that 
defaults to false. Just because its mainly used, doesn't mean there aren't some 
projects that prefer to use `enum struct`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal updated this revision to Diff 287478.
janosbenjaminantal added a comment.

Fix clang-tidy issues.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp
@@ -0,0 +1,165 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-prefer-scoped-enums %t --
+
+enum ForwardDeclaredUnscopedEnumUnsatisfied : int;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnumUnsatisfied' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnumUnsatisfied : int;{{$}}
+
+enum ForwardDeclaredUnscopedEnum : int;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnum : int;{{$}}
+
+enum ForwardDeclaredUnscopedEnum : int {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  // CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnum : int {{{$}}
+  FWUE_FirstValue,
+  FWUE_SecondValue,
+};
+
+const auto unqualifiedFWUE = FWUE_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:30: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}const auto unqualifiedFWUE = ForwardDeclaredUnscopedEnum::FWUE_FirstValue;{{$}}
+
+const auto qualifiedFWUE = ForwardDeclaredUnscopedEnum::FWUE_SecondValue;
+
+using AliasedFDUE = ForwardDeclaredUnscopedEnum;
+const auto qualifiedAFWUE = AliasedFDUE::FWUE_SecondValue;
+
+enum UnscopedEnum {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  // CHECK-FIXES: {{^}}enum class UnscopedEnum {{{$}}
+  UE_FirstValue,
+  UE_SecondValue,
+};
+
+auto qualifiedUE = UnscopedEnum::UE_FirstValue;
+
+auto unqualifiedUE = UE_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:22: warning: enumeration 'UnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}auto unqualifiedUE = UnscopedEnum::UE_FirstValue;{{$}}
+
+enum UnscopedEnumWithFixedUnderlyingType : char {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnumWithFixedUnderlyingType' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  UEWFUT_FirstValue,
+  UEWFUT_SecondValue,
+};
+// CHECK-FIXES: {{^}}enum class UnscopedEnumWithFixedUnderlyingType : char {{{$}}
+
+enum OpaqueUnscopedEnum : char;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'OpaqueUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class OpaqueUnscopedEnum : char;{{$}}
+
+enum UnfixableBecauseCast {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  UBC_FirstValue,
+  UBC_SecondValue,
+};
+
+const int castedUBC = UBC_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:23: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+
+const auto binaryOrUBC = UBC_FirstValue | UBC_SecondValue;
+// CHECK-NOTES: :[[@LINE-1]]:26: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-NOTES: :[[@LINE-2]]:43: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// notes about implicit casts are prompted after all of the warnings for the enums
+// CHECK-NOTES: :[[@LINE-7]]:23: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+// CHECK-NOTES: :[[@LINE-5]]:26: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+// CHECK-NOTES: :[[@LINE-6]]:43: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+
+enum UnfixableBecauseCastDefaultArgument {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal updated this revision to Diff 287461.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp
@@ -0,0 +1,165 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-prefer-scoped-enums %t --
+
+enum ForwardDeclaredUnscopedEnumUnsatisfied : int;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnumUnsatisfied' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnumUnsatisfied : int;{{$}}
+
+enum ForwardDeclaredUnscopedEnum : int;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnum : int;{{$}}
+
+enum ForwardDeclaredUnscopedEnum : int {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  // CHECK-FIXES: {{^}}enum class ForwardDeclaredUnscopedEnum : int {{{$}}
+  FWUE_FirstValue,
+  FWUE_SecondValue,
+};
+
+const auto unqualifiedFWUE = FWUE_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:30: warning: enumeration 'ForwardDeclaredUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}const auto unqualifiedFWUE = ForwardDeclaredUnscopedEnum::FWUE_FirstValue;{{$}}
+
+const auto qualifiedFWUE = ForwardDeclaredUnscopedEnum::FWUE_SecondValue;
+
+using AliasedFDUE = ForwardDeclaredUnscopedEnum;
+const auto qualifiedAFWUE = AliasedFDUE::FWUE_SecondValue;
+
+enum UnscopedEnum {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  // CHECK-FIXES: {{^}}enum class UnscopedEnum {{{$}}
+  UE_FirstValue,
+  UE_SecondValue,
+};
+
+auto qualifiedUE = UnscopedEnum::UE_FirstValue;
+
+auto unqualifiedUE = UE_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:22: warning: enumeration 'UnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}auto unqualifiedUE = UnscopedEnum::UE_FirstValue;{{$}}
+
+enum UnscopedEnumWithFixedUnderlyingType : char {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnumWithFixedUnderlyingType' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  UEWFUT_FirstValue,
+  UEWFUT_SecondValue,
+};
+// CHECK-FIXES: {{^}}enum class UnscopedEnumWithFixedUnderlyingType : char {{{$}}
+
+enum OpaqueUnscopedEnum : char;
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'OpaqueUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-FIXES: {{^}}enum class OpaqueUnscopedEnum : char;{{$}}
+
+enum UnfixableBecauseCast {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+  UBC_FirstValue,
+  UBC_SecondValue,
+};
+
+const int castedUBC = UBC_FirstValue;
+// CHECK-NOTES: :[[@LINE-1]]:23: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+
+const auto binaryOrUBC = UBC_FirstValue | UBC_SecondValue;
+// CHECK-NOTES: :[[@LINE-1]]:26: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// CHECK-NOTES: :[[@LINE-2]]:43: warning: enumeration 'UnfixableBecauseCast' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums]
+// notes about implicit casts are prompted after all of the warnings for the enums
+// CHECK-NOTES: :[[@LINE-7]]:23: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+// CHECK-NOTES: :[[@LINE-5]]:26: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+// CHECK-NOTES: :[[@LINE-6]]:43: note: implicit cast prevents 'UnfixableBecauseCast' from being fixed
+
+enum UnfixableBecauseCastDefaultArgument {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: enumeration 'UnfixableBecauseCastDefaultArgument' is not a scoped enumeration 

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal added a comment.

I addressed all of the review comments. Apart from the small fixes I extended a 
checker logic.

The check collects the following information from every unscoped enum (into the 
`FoundEnumInfo` struct):

1. **The enum's definition**: Every information from an `enum` is attached to 
the `enum` definition. If an `enum` is not defined in the translation unit, 
then the first forward declaration will represent the `enum`.
2. **Whether the enum definition can be fixed**: if the enum definition appears 
in a macro, then it cannot be fixed. otherwise it can be fixed by inserting the 
`class` keyword into the definition.
3. **Fixable usages of `enum` values**: these are the usages that can be fixed 
by inserting the right qualifier.
4. **Fixable forward declarations of the `enum`**: these can and must be fixed 
by inserting the `class` keyword similarly to the definition.
5. **Usages that cannot be fixed**: these are the usages that are in a macro.
6. **Forward declarations that cannot be fixed**: similarly to the unfixable 
usages, these are in a macro.
7. **Implicit casts**: the implicit casts prevent the `enum` from being fixed, 
because the scoped enumerations cannot be implicitly casted to numerical types.

If any of the definition/usages/forward declarations/implicit casts prevent the 
`enum` from being fixed, a note is prompted out for every occurrences to inform 
the users about this.

I though about more cases that can prevent an `enum` from being fixed, but I 
haven't found any other reason. If you know anything else, feel free to mention 
it.

I tried to come up with relevant, complex and stressful test cases. I already 
spot a few implementation bugs with them. If you miss anything, feel free to 
let me know.

The known "limitations" are mostly similar to other checks:

- It cannot detect unfixable cases from other translation units. Practically 
that means if an `enum` is used in multiple source files, one of them might end 
up not fixed. I tried to work around this, but I haven't found any solution for 
this, moreover this cause problems for other checks also. Therefore I think it 
shouldn't be a blocking issue.
- It doesn't take into account if an `enum` is defined in a header that is 
filtered out. Similarly to the previous one, I tried to find a solution for 
this, but I was unable (the `ClangTidyCheck` class can access the header filter 
information, but it doesn't expose it for the descendant classes). I also 
checked other checks, and they behave in the same way. Therefore I also think 
it is shouldn't be a blocking issue.
- The checks doesn't take into consideration the aliases in the diagnostic 
messages and fixes: always the original name is showed/inserted. I checked 
other checks also, and for the diagnostics message they also show the original 
name. I didn't find a check where the alias can be relevant when the fixes are 
applied. This issue might be a problem, however in my opinion it is not serious 
issue. Either it is okay for you or not, if you know how it could be improved, 
please provide me some points where to start searching. Even if this patch is 
got approved, I plan to improve it in the future and this point is a good 
things to improve.

It is not strongly connected to this review, but in the future I am planning to 
extend the check with:

- options to exclude enums, because changing them to scoped enumerations might 
not be suitable for every cases
- options to force the doable fixes: based on my little experience it might be 
easier to force the doable fixes and manually fix the remaining ones

If you have any opinion, thoughts or counter-argument against the planned 
extensions, please let me know.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal added a comment.

In D85697#2209272 , @njames93 wrote:

> Is 'over-unscoped' really needed in the name, would just 'prefer-scoped-enum' 
> be better, WDYT?
>
> For the case of macro, you can check if the location is a macro using 
> `SourceLocation::isMacroID()`.
>
> For this to work you would also need to check every single usage of the the 
> values in the enum to make sure they are converted to use the scoped access.
> You're best bet would be to actually store a map indexed by unscoped enum 
> decls with a set of all their locations and maybe a flag to say if the fix 
> can be applied or not.
> For instance a fix couldn't be applied if any of the usages or decls are in 
> macros.
> This map could then be checked using the `endOfTranslationUnit` virtual 
> method, with all the diags and fixes being spawned there.

Yes, it is better without 'over-unscoped'. I will change this.

I checked how other checks are dealing with macros, but I haven't found 
anything useful. Your idea sounds good, I will try to implement it.

- Fix not just the declarations, but also the enum usages
- Fix only the enums that are not declared/used in macros

As I am totally new to llvm, it might take a few days to come with a proper 
solutions, but I will do my best.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Is 'over-unscoped' really needed in the name, would just 'prefer-scoped-enum' 
be better, WDYT?

For the case of macro, you can check if the location is a macro using 
`SourceLocation::isMacroID()`.

For this to work you would also need to check every single usage of the the 
values in the enum to make sure they are converted to use the scoped access.
You're best bet would be to actually store a map indexed by unscoped enum decls 
with a set of all their locations and maybe a flag to say if the fix can be 
applied or not.
For instance a fix couldn't be applied if any of the usages or decls are in 
macros.
This map could then be checked using the `endOfTranslationUnit` virtual method, 
with all the diags and fixes being spawned there.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsOverUnscopedCheck.cpp:31-32
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("enumDecls");
+  if (MatchedDecl->isScoped())
+return;
+  diag(MatchedDecl->getLocation(), "enumeration %0 is not a scoped 
enumeration")

Please hoist this logic into the matcher.
```
enumDecl(unless(isScoped())).bind("enumDecls")



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst:20
+B
+  }
+

Semi-colon after the struct definition.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst:29
+B
+  }
+

Semi-colon after the struct definition.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums-over-unscoped.cpp:27-28
+enum struct ScopedEnumWithStruct {
+  SEWC_FirstValue,
+  SEWC_SecondValue,
+};

nit:


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new check in Release Notes.




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst:22
+
+After the fix is applied, the enum will become:
+

Please enclose enum in double back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst:65
+  CREATE_ENUM(class Foo);
\ No newline at end of file


Please add newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal created this revision.
janosbenjaminantal added reviewers: alexfh, njames93, xazax.hun, Eugene.Zelenko.
Herald added subscribers: cfe-commits, aaron.ballman, rnkovacs, kbarton, 
mgorny, nemanjai.
Herald added a project: clang.
janosbenjaminantal requested review of this revision.
Herald added a subscriber: wuzish.

This check aims to flag every occurence of unscoped enumerations and provide 
useful fix to convert them to scoped enumerations.

It works for the most cases, except when an enumeration is defined within a 
macro, but the name of enumeration is a macro argument. This is indicated in 
the documentation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85697

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsOverUnscopedCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsOverUnscopedCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums-over-unscoped.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums-over-unscoped.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums-over-unscoped.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-prefer-scoped-enums-over-unscoped %t --
+
+enum UnscopedEnum {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums-over-unscoped]
+  UE_FirstValue,
+  UE_SecondValue,
+};
+// CHECK-FIXES: {{^}}enum class UnscopedEnum {{{$}}
+
+enum UnscopedEnumWithFixedUnderlyingType : char {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enumeration 'UnscopedEnumWithFixedUnderlyingType' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums-over-unscoped]
+  UEWFUT_FirstValue,
+  UEWFUT_SecondValue,
+};
+// CHECK-FIXES: {{^}}enum class UnscopedEnumWithFixedUnderlyingType : char {{{$}}
+
+enum OpaqueUnscopedEnum : char;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enumeration 'OpaqueUnscopedEnum' is not a scoped enumeration [cppcoreguidelines-prefer-scoped-enums-over-unscoped]
+// CHECK-FIXES: {{^}}enum class OpaqueUnscopedEnum : char;{{$}}
+
+enum class ScopedEnumWithClass {
+  SEWC_FirstValue,
+  SEWC_SecondValue,
+};
+
+enum struct ScopedEnumWithStruct {
+  SEWC_FirstValue,
+  SEWC_SecondValue,
+};
+
+enum class OpaqueScopedEnum;
+
+enum class OpaqueScopedEnumWithFixedUnderlyingType : unsigned;
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -141,6 +141,7 @@
`cppcoreguidelines-narrowing-conversions `_,
`cppcoreguidelines-no-malloc `_,
`cppcoreguidelines-owning-memory `_,
+   `cppcoreguidelines-prefer-scoped-enums-over-unscoped `_, "Yes"
`cppcoreguidelines-pro-bounds-array-to-pointer-decay `_,
`cppcoreguidelines-pro-bounds-constant-array-index `_, "Yes"
`cppcoreguidelines-pro-bounds-pointer-arithmetic `_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst
@@ -0,0 +1,64 @@
+.. title:: clang-tidy - cppcoreguidelines-prefer-scoped-enums-over-unscoped
+
+cppcoreguidelines-prefer-scoped-enums-over-unscoped
+===
+
+Values of unscoped enumerations are implicitly-convertible to integral types.
+To avoid such unwanted conversions, use scoped enumerations.
+
+This check implements 
+`Enum.3 `_ 
+from the CppCoreGuidelines.
+
+Example:
+
+.. code-block:: c++
+
+  enum Foo {
+A,
+B
+  }
+
+After the fix is applied, the enum will become:
+
+.. code-block:: c++
+
+  enum class Foo {
+A,
+B
+  }
+
+Note: Although ``enum struct`` and ``enum class`` are exactly equivalent, the
+latter is used mainly.
+
+Limitations
+===
+
+If the enumeration is generated via a macro and the enumeration name is
+received as a parameter, then the suggested fix would append the ``class``
+word before the name of enumeration at the macro invocation instead of the
+macro definition.
+
+Example:
+
+.. code-block:: c++
+
+  #define CREATE_ENUM(Name) \
+  enum Name {   \
+A,  \
+B   \