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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
