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

Mon Oct 19 16:13:11 PDT 2020

janosbenjaminantal added a comment.

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

> In D85697#2234468 <https://reviews.llvm.org/D85697#2234468>, @janosbenjaminantal wrote:
>
>> The known "limitations" are mostly similar to other checks:
>>
>> - It cannot detect unfixable cases from other translation units. Practically that means if an `enum` is used in multiple source files, one of them might end up not fixed. I tried to work around this, but I haven't found any solution for this, moreover this cause problems for other checks also. Therefore I think it shouldn't be a blocking issue.
>
> That's what the run_clang_tidy.py script is meant to handle.

It is good to know, so it is not an issue.

>> - It doesn't take into account if an `enum` is defined in a header that is filtered out. Similarly to the previous one, I tried to find a solution for this, but I was unable (the `ClangTidyCheck` class can access the header filter information, but it doesn't expose it for the descendant classes). I also checked other checks, and they behave in the same way. Therefore I also think it is shouldn't be a blocking issue.
>
> I recently pushed an upgrade to readability-identifier-naming where it would check the naming style for identifiers declared in header files, maybe thats something this could also use, this is the commit 4888c9ce97d8 <https://reviews.llvm.org/rG4888c9ce97d8c20d988212b10f1045e3c4022b8e>

Will check.

>> It is not strongly connected to this review, but in the future I am planning to extend the check with:
>>
>> - options to exclude enums, because changing them to scoped enumerations might not be suitable for every cases
>
> Not strictly necessary, if people don't want the fix they could annotate the code with a `// NOLINT(*prefer-unscoped-enums)` comment.

I think with the inline suppression the users should annotate every usage of the enum while with the option it would be enough to list the enum's name to ignore it from the whole check. However it is just an improvement, when I reach that point I will do further digging about this topic, how the suppression actually work etc.

>> - options to force the doable fixes: based on my little experience it might be easier to force the doable fixes and manually fix the remaining ones
>
> Forcing the fix is usually just a case of converting implicit cast usages of the constants into static casts.

What I meant is an option to express that the user don't care if there are some cases for an enum that cannot be fixed: fixed the other occurrences and the user will handle the rest. Currently the automated fix only fix the enums where every occurrence can be fixed. As a second thought this might be superfluous, because the other way is always possible: the user first fix the occurrences that cannot be fixed automatically and then use the check to fix the rest. So I think it is something that wouldn't mean real value.

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

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

================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsCheck.h:33
+
+  void onEndOfTranslationUnit() override;
+
----------------
aaron.ballman wrote:
> Does this need to be `public`?
The original function in MatchCallback is public, so I think it is reasonable to have it as a public function here also.

================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums.cpp:33
+};
+
+auto qualifiedUE = UnscopedEnum::UE_FirstValue;
----------------
aaron.ballman wrote:
> 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`.
That's an excellent catch. I think comparing the size of `int` and the underlying type of the `enum` might help with some extra logic, because

```
enum class ValidWithLongLongInt {
  Value = 0x7FFF'FFFF'FFFF'FFFF
};

enum InvalidWithLongLongInt {
  Value = 0x8FFF'FFFF'FFFF'FFFF
};
```
The second one must use unsigned int. I think this is manageable and will have a look at it.

================
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);
> ```
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697