Page MenuHomePhabricator

[clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped
Needs ReviewPublic

Authored by janosbenjaminantal on Aug 10 2020, 5:08 PM.

Details

Summary

This check aims to flag every occurence of unscoped enumerations and provide useful fix to convert them to scoped enumerations.

It works for the most cases, except when an enumeration is defined within a macro, but the name of enumeration is a macro argument. This is indicated in the documentation.

Diff Detail

Event Timeline

janosbenjaminantal requested review of this revision.Aug 10 2020, 5:08 PM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko removed a subscriber: aaron.ballman.

Please mention new check in Release Notes.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst
22 ↗(On Diff #284531)

Please enclose enum in double back-ticks.

65 ↗(On Diff #284531)

Please add newline.

Is 'over-unscoped' really needed in the name, would just 'prefer-scoped-enum' be better, WDYT?

For the case of macro, you can check if the location is a macro using SourceLocation::isMacroID().

For this to work you would also need to check every single usage of the the values in the enum to make sure they are converted to use the scoped access.
You're best bet would be to actually store a map indexed by unscoped enum decls with a set of all their locations and maybe a flag to say if the fix can be applied or not.
For instance a fix couldn't be applied if any of the usages or decls are in macros.
This map could then be checked using the endOfTranslationUnit virtual method, with all the diags and fixes being spawned there.

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferScopedEnumsOverUnscopedCheck.cpp
31–32 ↗(On Diff #284531)

Please hoist this logic into the matcher.

enumDecl(unless(isScoped())).bind("enumDecls")`
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-scoped-enums-over-unscoped.rst
20 ↗(On Diff #284531)

Semi-colon after the struct definition.

29 ↗(On Diff #284531)

Semi-colon after the struct definition.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-scoped-enums-over-unscoped.cpp
27–28 ↗(On Diff #284531)

nit:

Is 'over-unscoped' really needed in the name, would just 'prefer-scoped-enum' be better, WDYT?

For the case of macro, you can check if the location is a macro using SourceLocation::isMacroID().

For this to work you would also need to check every single usage of the the values in the enum to make sure they are converted to use the scoped access.
You're best bet would be to actually store a map indexed by unscoped enum decls with a set of all their locations and maybe a flag to say if the fix can be applied or not.
For instance a fix couldn't be applied if any of the usages or decls are in macros.
This map could then be checked using the endOfTranslationUnit virtual method, with all the diags and fixes being spawned there.

Yes, it is better without 'over-unscoped'. I will change this.

I checked how other checks are dealing with macros, but I haven't found anything useful. Your idea sounds good, I will try to implement it.

  • Fix not just the declarations, but also the enum usages
  • Fix only the enums that are not declared/used in macros

As I am totally new to llvm, it might take a few days to come with a proper solutions, but I will do my best.

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.

Fix clang-tidy issues.