[PATCH] D84924: [clang-tidy] Added command line option `fix-notes`

Mon Dec 21 11:31:08 PST 2020

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