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

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

janosbenjaminantal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:91
+  llvm::SmallString<128> Buffer;
+  const StringRef ClassInsertion{"class "};
+

aaron.ballman wrote:
> janosbenjaminantal wrote:
> > aaron.ballman wrote:
> > > This feels a bit like it's a user option given that `enum struct` is also 
> > > used. I think using `class` is a sensible default, though.
> > I would like to keep this as small as possible, so I didn't planned to add 
> > this option as part of the first version. Do you think it is necessary to 
> > add this to approve the review? If yes, then I will add it of course, 
> > otherwise maybe in a different review in the future.
> Given that two reviewers both thought it was worthwhile, I think it's 
> probably not a bad idea to support as an option. It could be done in a 
> follow-up patch if that's the way you'd like to proceed, though.
Will implement this. Not sure whether in a follow-up patch or not. If it is 
just a few lines of code, then I will include here, otherwise I will create a 
follow-up patch.


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

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

janosbenjaminantal added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:165
+
+enum class OpaqueScopedEnumWithFixedUnderlyingType : unsigned;

aaron.ballman wrote:
> njames93 wrote:
> > janosbenjaminantal wrote:
> > > aaron.ballman wrote:
> > > > What should happen in cases like:
> > > > ```
> > > > // A case where the user has manually added a prefix they want; it
> > > > // seems like fixing this to use a scoped enumeration changes the
> > > > // expected way you interface with the enumeration by name.
> > > > namespace EnumPrefix {
> > > > enum Whatever {
> > > >   One,
> > > >   Two
> > > > };
> > > > }
> > > > 
> > > > // Another form of the same idea above.
> > > > struct AnotherPrefix {
> > > >   enum Whatever {
> > > > One,
> > > > Two
> > > >   };
> > > > };
> > > > 
> > > > // What about use in class hierarchies?
> > > > // Header file the user controls
> > > > enum SomeEnum {
> > > >   One,
> > > >   Two
> > > > };
> > > > 
> > > > struct SomeType {
> > > >   virtual void func(SomeEnum E) = 0; // Changing this may break the 
> > > > hierarchy
> > > > };
> > > > 
> > > > // Here's another situation where even warning is a bit suspect
> > > > enum E : int;
> > > > extern void func(E);
> > > > ```
> > > I think there is no good solution for the first two cases. We cannot be 
> > > sure whether it is meant to be a prefix or not. It is just a very 
> > > educated guess (if the the scope contains only one enum, then it is very 
> > > like just a prefix, but might not be true in every cases). I don't have a 
> > > very strong opinion about this, but I think this check can be useful even 
> > > without supporting this case. Do you think it is a very common solution?
> > > 
> > > For the third one, I think if the fix is applied for every files, then it 
> > > won't be a problem. If the fix cannot be applied to all of the files 
> > > (e.g.: in case of a lib where the users can inherit from the classes 
> > > defined in the lib) it will definitely break the hierarchy, however I 
> > > don't think we can do anything about this. I am not totally sure, but I 
> > > think this a problem for a lot of other checks too, e.g.: 
> > > google-explicit-constructor or readability-identifier-naming. However I 
> > > am afraid I misunderstood something, so let me in that case.
> > > 
> > > The last case with the extern function is a perfect place to suppress the 
> > > warning in my opinion. How it could be decided, whether the user can 
> > > change the function or not? Therefore I would keep the warning in that 
> > > case and trust the users to suppress it if necessary.
> > Maybe the check should only flag enumerations that appear at TU level, any 
> > other enumeration is kind of scoped already.
> >  Do you think it is a very common solution?
> 
> I think it likely is -- it was a common tactic used in pre-C++11 code bases 
> (for instance, Clang uses this quite often, as in: 
> https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Decl.h#L837)
> 
> > For the third one, I think if the fix is applied for every files, then it 
> > won't be a problem. 
> 
> Agreed -- the scenario I was thinking about is when the virtual hierarchy is 
> part of a published interface, like in a plugin system. Changing the base 
> declaration may be easy, but you may not even *see* the other derived 
> classes. I'm not certain it's a solvable issue.
> 
> > The last case with the extern function is a perfect place to suppress the 
> > warning in my opinion.
> 
> Agreed.
> 
> > Maybe the check should only flag enumerations that appear at TU level, any 
> > other enumeration is kind of scoped already.
> 
> I was sort of wondering the same thing, but perhaps instead of TU level, I'd 
> go with TU or anonymous namespace so that we still diagnose things like these:
> ```
> namespace {
> enum E { One }; // Seems reasonable to suggest scoping
> namespace {
> enum F { Two }; // Same here even though there are nested namespaces
> }
> }
> ```
I think the scoped vs unscoped enumeration is not about the scope itself, more 
likely the conversion rules applied to them: the unscoped enumerations are 
easily, even mistakenly convertible to/from `int`s. The core guideline reasons 
with this: 
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class

From that point of view, it is reasonable to convert the enums that are in 
their own namespaces also. Of course, it would be nice to eliminate the extra 
scope, but I don't think we can detect it. From my understanding, the only we 
could do is just check whether and enumeration is the only thing in the 
namespace, and if yes, then remove the extra namespace. However I think this is 
not very reliable.

I will try to find a pre C++11 projects which use this and check there how it 
could be solved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

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

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

janosbenjaminantal added a comment.

In D85697#2339055 , @aaron.ballman 
wrote:

> I tend to be very skeptical of the value of checks that basically throw out 
> entire usable chunks of the base language because the check is effectively 
> impossible to apply to an existing code base. Have you run the check over 
> large C++ code bases to see just how often the diagnostic triggers and 
> whether there are ways we might want to reduce false-positives that the C++ 
> Core Guidelines haven't considered as part of their enforcement strategy?

No, I haven't checked over large C++ code bases, but I will do this.

> Btw, one fear I have with this check is that the automated fixits are 
> somewhat risky -- unscoped vs scoped enumerations has ABI implications and 
> changing from one to the other may break the ABI.

I am not familiar with these specific ABI implications, if you could help me 
with some keywords/references/link to start to investigate, I am happy to deep 
dive into it. I am also willing to discard the automated fixes if it makes this 
review better. What do you think?


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

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

janosbenjaminantal added a comment.

In D85697#2338249 , @njames93 wrote:

> 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 is good to know, so it is not an 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.
>
> 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 
> 

Will check.

>> 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.

I think with the inline suppression the users should annotate every usage of 
the enum while with the option it would be enough to list the enum's name to 
ignore it from the whole check. However it is just an improvement, when I reach 
that point I will do further digging about this topic, how the suppression 
actually work etc.

>> - 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.

What I meant is an option to express that the user don't care if there are some 
cases for an enum that cannot be fixed: fixed the other occurrences and the 
user will handle the rest. Currently the automated fix only fix the enums where 
every occurrence can be fixed. As a second thought this might be superfluous, 
because the other way is always possible: the user first fix the occurrences 
that cannot be fixed automatically and then use the check to fix the rest. So I 
think it is something that wouldn't mean real value.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:22
+const LangOptions ) const {
+  return LangOpts.CPlusPlus11;
+}

njames93 wrote:
> aaron.ballman wrote:
> > FWIW, Clang accepts scoped enumerations as a conforming language extension 
> > in older language modes. Should this check be enabled for any C++ mode?
> If you go down that route, you lose portability as the transformed code will 
> only compile on versions of clang, Could be option controlled I guess, WDYT?
I think by default the check shouldn't do that (source codes that uses multiple 
compilers?), but it might worth an option to be able to turn it on on 
"clang-only" code bases.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:91
+  llvm::SmallString<128> Buffer;
+  const StringRef ClassInsertion{"class "};
+

aaron.ballman wrote:
> This feels a bit like it's a user option given that `enum struct` is also 
> used. I think using `class` is a sensible default, though.
I would like to keep this as small as possible, so I didn't planned to add this 
option as part of the first version. Do you think it is necessary to add this 
to approve the review? If yes, then I will add it of course, otherwise maybe in 
a different review in the future.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h:33
+
+  void onEndOfTranslationUnit() override;
+

aaron.ballman wrote:
> Does this need to be `public`?
The original function in MatchCallback is public, so I think it is reasonable 
to have it as a public function here also.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:33
+};
+
+auto qualifiedUE = UnscopedEnum::UE_FirstValue;

aaron.ballman wrote:
> Another interesting test case:
> ```
> enum 

[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

[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   \