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

Tue Oct 20 10:55:07 PDT 2020

aaron.ballman added a comment.

In D85697#2340258 <https://reviews.llvm.org/D85697#2340258>, @njames93 wrote:

> In D85697#2339055 <https://reviews.llvm.org/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?
>
> I disagree with this mentality, This is following what the core guideline states perfectly. If a codebase doesn't want to adhere to those guidelines, they don't need to run this check. If you think there are shortcomings in the guidelines, raise an issue on their github.

As with other C++ core guideline checks, the usual approach is for the patch authors to go back to the C++ Core Guideline folks with these issues to find out how they should be addressed. This has produced better C++ Core Guidelines and better checkers in the past and is the way I think we should continue to proceed. That said, I agree with you that we will eventually go with whatever the guideline authors want -- but this is us doing our due diligence since the authors put very little thought into enforcement of the rules in general, and is appropriate (for any guidelines, not just the C++ core guidelines). Note, this is in keeping with the guidelines themselves which state that they're not meant to subset the language and are intended to allow for gradual adoption of rules (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#innot-non-aims).

In D85697#2340381 <https://reviews.llvm.org/D85697#2340381>, @janosbenjaminantal wrote:

> In D85697#2339055 <https://reviews.llvm.org/D85697#2339055>, @aaron.ballman wrote:
>
>> 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?

I did some digging and I'm less certain I'm worried about this now. I was thinking that the mangling was different for these two cases, but it's the same mangling for `func()` either way: https://godbolt.org/z/xhcd7E The automated fixits still worry me in the places where they may take working code and change it to not be working (like narrowing conversions with the enumerator values, etc), but if those are addressed, then I think the fix-its are a benefit rather than a risk.

================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:22
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
----------------
janosbenjaminantal wrote:
> 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.
The impression I have from the guidelines is that they're intended for C++11 and later code bases as a way to improve the coding practices. They don't really seem to imply there's a requirement that code remains portable, but that sure seems like a reasonable goal to me. I think I'm fine leaving this disabled unless you're in C++11 mode, but we may want to see if there's a policy decision here for the core guideline checks (I have no idea how consistent we're being with considering older standards for these particular checks).

================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:28
+
+  Finder->addMatcher(UnscopedEnumDecl().bind("enumDecl"), this);
+
----------------
njames93 wrote:
> aaron.ballman wrote:
> > This should be checking whether the enumeration comes from a system header or not -- we should definitely not warn about enumerations the user cannot change.
> > This should be checking whether the enumeration comes from a system header or not -- we should definitely not warn about enumerations the user cannot change.
> 
> Won't clang-tidy handle this by suppressing system header diagnostics, I thought that's what the feature was there for.
Good point! I forgot that clang-tidy already handled that automatically.

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

================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:165
+
+enum class OpaqueScopedEnumWithFixedUnderlyingType : unsigned;
----------------
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
}
}
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697