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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:22
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:28
+
+  Finder->addMatcher(UnscopedEnumDecl().bind("enumDecl"), this);
+
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:165
+
+enum class OpaqueScopedEnumWithFixedUnderlyingType : unsigned;
----------------
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);
> ```
Maybe the check should only flag enumerations that appear at TU level, any other enumeration is kind of scoped already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697