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

Tue Oct 20 05:24:11 PDT 2020

njames93 added a comment.

In D85697#2340317 <https://reviews.llvm.org/D85697#2340317>, @janosbenjaminantal wrote:

> In D85697#2338249 <https://reviews.llvm.org/D85697#2338249>, @njames93 wrote:
>
>> In D85697#2234468 <https://reviews.llvm.org/D85697#2234468>, @janosbenjaminantal wrote:
>>
>>> 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.

Ah so an issue here is you emit a warning for the enum declaration as well as each usage and the NOLINT will only affect warnings emitted for the line its declared on.
Perhaps this needs a reshape.

Only emit a warning for the enum declaration and make sure the fix-its are attached to that warning, This is a safer way around this, and its also how RenamerClangTidy checks work. It means that if there is any conflict when trying to apply one of the fix-its none of them will be applied.
If you want to emit notes about usages that need updating you can, but don't attach fixes there, they wont be applied.

Due to how diagnostics works this will make it the code slightly more finicky, you can only have one DiagnosticBuilder active at once, and the notes need to be added after the warning has been emitted.

Maybe if it can be fixed, Emit a warning for the enum declaration, then add fix-its for the decl, any forward declares and any usages, then emit that diagnostic (Happens when the DiagnosticBuilder gets destroyed). After that you could emit notes for the forward declarations and usages(though I don't think we really needs notes for those)
If it can't be fixed its kind of similar, emit a warning for the declaration, then a note for each forward declaration and fault for why a fix can't be applied.
This has the added bonus that clang-tidy will output the diagnostics for an unscoped enum and all its uses next to each other.
If you want I've done the legwork to change the diagnostics behaviour to this and fix up the test cases if you want to have a look, should apply cleanly atop this version.
F13390481: ScopedEnums.patch <https://reviews.llvm.org/F13390481>

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85697