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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?

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.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:22
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
----------------
FWIW, Clang accepts scoped enumerations as a conforming language extension in older language modes. Should this check be enabled for any C++ mode?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:26
+void PreferScopedEnumsCheck::registerMatchers(MatchFinder *Finder) {
+  const auto UnscopedEnumDecl = []() { return enumDecl(unless(isScoped())); };
+
----------------
Drop top-level `const` qualifiers on things that are not pointers or references (it's not a pattern we typically use in the project). This applies elsewhere in the patch as well.


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


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


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:94-95
+  for (const auto &EnumDeclAndInfo : UnscopedEnumInfos) {
+    const auto *UnscopedEnumDecl = EnumDeclAndInfo.first;
+    const auto &UnscopedEnumInfo = EnumDeclAndInfo.second;
+
----------------
Please do not use `auto` when the type is not spelled out in the initialization (or is blindingly obvious from context, like iterators, or is unutterable, like lambdas).


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:124
+
+      for (const auto *const EnumUsage : UnscopedEnumInfo.Usages)
+        DiagUsageWarning(EnumUsage, UnscopedEnumDecl);
----------------
I would drop the top-level `const` pointer qualification (elsewhere as well). The object being pointed at is fine, I'm only suggesting removing the qualification for the pointer itself.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:134
+
+      for (const auto *const ForwardDecl : UnscopedEnumInfo.ForwardDecls)
+        DiagDeclWarning(ForwardDecl);
----------------
`llvm::for_each()`? This may actually shorten many of the loops in this function.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:167
+  } else if (const auto *Definition = UnscopedEnumDecl->getDefinition()) {
+    auto &EnumInfo = UnscopedEnumInfos[Definition];
+
----------------
Please don't use `auto` here.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.cpp:194
+    return UnscopedEnumDecl->getTypeForDecl()->getCanonicalTypeInternal() !=
+           Qualifier->getAsType()->getCanonicalTypeInternal();
+  }();
----------------
Why are you using the internal interfaces here?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h:33
+
+  void onEndOfTranslationUnit() override;
+
----------------
Does this need to be `public`?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h:38-42
+    std::vector<const DeclRefExpr *> Usages{};
+    std::vector<const EnumDecl *> ForwardDecls{};
+    std::vector<const DeclRefExpr *> UsagesPreventFix{};
+    std::vector<const EnumDecl *> ForwardDeclsPreventFix{};
+    std::vector<const ImplicitCastExpr *> ImplicitCasts{};
----------------
Please remove the unnecessary empty initializers.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums.rst:3
+
+cppcoreguidelines-prefer-scoped-enums
+===================================================
----------------
Underlining looks to be the wrong length.


================
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.
----------------
njames93 wrote:
> 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`.
+1. Also, we should document when the check doesn't trigger (like macros or system headers), discuss forward reference behavior, etc.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:33
+};
+
+auto qualifiedUE = UnscopedEnum::UE_FirstValue;
----------------
Another interesting test case:
```
enum BigEnumConstants {
  TooBig = 0xFFFF'FFFF'0
};
```
Changing this unscoped enumeration into a scoped enumeration without specifying the fixed underlying type will break code in this case because the enum constant value is too large to be represented by an `int`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697