[PATCH] D159263: [clang-tidy] misc-include-cleaner: avoid duplicated fixes

Tue Sep 5 06:05:07 PDT 2023

danix800 added a comment.

In D159263#4638167 <https://reviews.llvm.org/D159263#4638167>, @kadircet wrote:

> Thanks a lot for the patch @danix800 !
>
> I initially was rather focused on behavior of this check in workflows that require seeing "self-contained-diags", but also I rather see the bulk-apply use cases as always requiring clang-format afterwards. Hence didn't really try to polish that use case a lot, but I believe changes in this patch improve those use cases reasonably. But users do still need to run clang-format afterwards, e.g. if we first generate a finding that inserts "b.h" and then "a.h", they'll be in wrong order. So this is only fixing  some cases. If you'd like to work on a more complete approach, we can prepare all the edits in a single `tooling::Replacements` and run `clang::format::cleanupAroundReplacements` on top of it to generate proper edits, and emit FixIts with those merged replacements. Note that this still won't be enough in all cases, as there can be other checks that emit fixes that are touching includes :/
>
> Regarding usage of `IncludeInserter`; we were deliberately not using `IncludeInserter` here, as it has slightly different semantics to `HeaderIncludes`, which is used by all the other applications of include-cleaner when producing edits. That way it's easier for us to reason about the behavior in various places (and also to fix them). But moreover, `HeaderIncludes` uses clang-format config to figure out include-styles and works with a bigger set of projects without requiring extra configurations (hence this patch will actually be a regression for those). Therefore can you keep using `HeaderIncludes`, while skipping generation of duplicate fixits when we're in non-self-contained-diags mode (assuming you don't want to generalize the approach as I explained above).

Thanks for the background!

If we have further plans from upstream on these checker/clang-format related logic, I can wait and abandon this revision.

Or if such issue does need fixing then coud you take a look at the previous diff 1 <https://reviews.llvm.org/D159263?id=554947>
which uses an internal set to prevent this issue?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159263