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