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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> In D106431#2896334 <https://reviews.llvm.org/D106431#2896334>, @aaron.ballman wrote:
>
>> In D106431#2896206 <https://reviews.llvm.org/D106431#2896206>, @whisperity wrote:
>>
>>> The problem with enums is that translating //zero// (0, 0.0, nullptr, etc...) to the enum case is not always apparent. A warning **should** always be given. And //if// you can find a zero member in the enum, we can report an automated suggestion for that.
>>
>> I think we shouldn't give any fix-it for enumerations. Zero doesn't always mean "the right default value" -- for example, another common idiom is for the *last* member of the enumeration to be a sentinel value.
>
> I agree with you, but we need to consider that the checker works in a way that it gives the "zero" for integers. If we are here, was that the right decision?

If we're here, I think the right decision would have been to not support the C++ Core Guidelines in clang-tidy in the first place because of the lack of attention that's been paid to enforcement for the rules by the authors. ;-) But snark aside, I think `= 0` is a somewhat defensible naïve default value for integers, but today I would prefer it give its fix-it on a note rather than on the warning. When there's only one fix-it to be applied to a warning, it's possible to apply the fix-it without any per-case user intervention and that default value isn't statically known to be safe. With `nullptr` for pointers, it's more defensible because that's a well-understood sentinel value that code *should* be checking for (and there's good tooling to catch null pointer dereferences for the other situations). With `NAN` for floats, it's more defensible because that (usually!) is a "sticky" value that poisons further uses of the type. There's nothing quite like that for integers.

> I mean... I wonder how much //consistency// we should shoot for. (`nullptr` for the pointers as default is at least somewhat sensible.)

There are at least two ways to chase consistency here -- one is being consistent with other fix-its in the check, the other is being consistent with other fix-its across the project. My feeling is we should aim for consistency with the rest of the toolchain, which is to only issue a fix-it on a warning when the fix is known to be correct. https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints

Perhaps one way forward would be to issue the integer fix-it on a note rather than a warning, and 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.

Another way forward would be to not issue a fix-it for integers or enumerations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431