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

[János Benjamin Antal via Phabricator via cfe-commits]

janosbenjaminantal added a comment.

I addressed all of the review comments. Apart from the small fixes I extended a checker logic.

The check collects the following information from every unscoped enum (into the `FoundEnumInfo` struct):

1. **The enum's definition**: Every information from an `enum` is attached to the `enum` definition. If an `enum` is not defined in the translation unit, then the first forward declaration will represent the `enum`.
2. **Whether the enum definition can be fixed**: if the enum definition appears in a macro, then it cannot be fixed. otherwise it can be fixed by inserting the `class` keyword into the definition.
3. **Fixable usages of `enum` values**: these are the usages that can be fixed by inserting the right qualifier.
4. **Fixable forward declarations of the `enum`**: these can and must be fixed by inserting the `class` keyword similarly to the definition.
5. **Usages that cannot be fixed**: these are the usages that are in a macro.
6. **Forward declarations that cannot be fixed**: similarly to the unfixable usages, these are in a macro.
7. **Implicit casts**: the implicit casts prevent the `enum` from being fixed, because the scoped enumerations cannot be implicitly casted to numerical types.

If any of the definition/usages/forward declarations/implicit casts prevent the `enum` from being fixed, a note is prompted out for every occurrences to inform the users about this.

I though about more cases that can prevent an `enum` from being fixed, but I haven't found any other reason. If you know anything else, feel free to mention it.

I tried to come up with relevant, complex and stressful test cases. I already spot a few implementation bugs with them. If you miss anything, feel free to let me know.

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.
- 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.
- The checks doesn't take into consideration the aliases in the diagnostic messages and fixes: always the original name is showed/inserted. I checked other checks also, and for the diagnostics message they also show the original name. I didn't find a check where the alias can be relevant when the fixes are applied. This issue might be a problem, however in my opinion it is not serious issue. Either it is okay for you or not, if you know how it could be improved, please provide me some points where to start searching. Even if this patch is got approved, I plan to improve it in the future and this point is a good things to improve.

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
- 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

If you have any opinion, thoughts or counter-argument against the planned extensions, please let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697