[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

Tue Jul 27 19:27:22 PDT 2021

steven.zhang added a comment.

In D106431#2907688 <https://reviews.llvm.org/D106431#2907688>, @whisperity wrote:

> In D106431#2907002 <https://reviews.llvm.org/D106431#2907002>, @Sockke wrote:
>
>> Any thoughts?  : )
>
> First, let's first fix that we should still warn for the uninitialised `enum` case, without a FixIt. That's the issue at hand, right now, Clang-Tidy generates, as you identified, broken output. We can discuss the later steps after this is fixed. Please implement this logic, and update the patch, so we have a snapshot of how that would look like and the thing working.

+1

> Afterwards, as Aaron suggested:
>
> In D106431#2896441 <https://reviews.llvm.org/D106431#2896441>, @aaron.ballman wrote:
>
>> for enumerations, we could issue up to two fix-its on a note, one for the first and one for the last enumerator in an enumeration (and if the enum only contains one enumerator, there's only one fix-it to generate which suggests it could be on the warning rather than a note, but that seems like a lot of trouble for an unlikely scenario). However, I don't recall how clang-tidy interacts with fix-its on notes off the top of my head, so I'm making an assumption that clang-tidy's automatic fixit applying mode handles notes the same way as clang and we should double-check that assumption.
>
> This might require nontrivial changes to the check's code, and investigating the potential problem with automated fix-it application when multiple conflicting fix-its are given on a **note** (not a //warning// line).
>
> I think we all would be fine with only doing the first step (reintroduce the warning, without a fixit) in this patch, so it can be merged and hit the the mainline code quickly. The next step can be its own patch. 🙂
>
> ----
>
> In addition, I wager that adding a line about this fix to the release notes at `clang-tools-extra/docs/ReleaseNotes.rst` is a good option, I can imagine people having turned this check off due to it being broken on enums.

I think, the intention of the auto fix for such case is to make the program's behavior well-defined, not with some uninitialize value that cannot be predicted. It is ok as far as we select any enumerator to initialize it from my understanding. We should separate it into anther patch of cause.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431