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

Wed Oct 21 23:39:45 PDT 2020

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
  https://reviews.llvm.org/D85697/new/

https://reviews.llvm.org/D85697