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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
