aaron.ballman added a comment. In D84924#2465745 <https://reviews.llvm.org/D84924#2465745>, @njames93 wrote:
> In D84924#2446075 <https://reviews.llvm.org/D84924#2446075>, @aaron.ballman > wrote: > >> In D84924#2184132 <https://reviews.llvm.org/D84924#2184132>, @njames93 wrote: >> >>> This is very much a work in progress >>> Another direction I was thinking was only apply the fixes found in notes if >>> there is exactly one fix attached to the notes in a diagnostic, instead of >>> just applying the first one we find >> >> I was wondering the same thing here myself. If there's exactly one fix, then >> it's unambiguous as to what behavior you get. One (minor) concern I have >> about the current approach is with nondeterminism in diagnostic ordering >> where different runs may pick different fixes for the same code. I don't >> *think* we have any instances of this in Clang or clang-tidy, but users can >> add their own plugins (for instance to the clang static analyzer) that may >> introduce some small risk there. Do you have a reason why you picked one >> approach over the other? > > Part of the reason for this approach is from this bug report > https://bugs.llvm.org/show_bug.cgi?id=47971, where its pointed out that clang > diags gets fix-its added in notes if the fixes will change behaviour or they > aren't sure its going to actually fix the issue. > As clang-tidy also applies fixes reported from clang, it is wise to adhere to > a similar level caution. Agreed, but I don't think the current behavior in this patch is cautious enough. I like that we have to specify "please apply fixits from notes" explicitly. I think that's the right behavior we want. But if there are multiple fixits attached to the notes for a diagnostic, I don't think we should assume that the first fix-it is the correct one to apply -- I think the user should have to manually pick the variant they want in that case. So I think the behavior should be to apply the fixits from notes if there's only a single fixit to be applied. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84924/new/ https://reviews.llvm.org/D84924 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
