[PATCH] D159263: [clang-tidy] misc-include-cleaner: remove duplicated includes & fixes

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! @kadircet should make the call on these features (they look good to me), I have some implementation notes.

This is making two independent changes, one is a policy change that should be applied to the include-cleaner library, and one is a bugfix to the clang-tidy check. I think they should be separate patches.



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:169
+      else
+        Unused.push_back(&I);
+      continue;
----------------
If we want this policy, it should be provided at the include-cleaner library level so it applies to all clients, not just the clang-tidy check.

I think the clearest way is to encapsulate this in include_cleaner::Includes, so that match() never matches a Header requirement against a duplicative Include. This will naturally lead to the directives being marked as unused.

(In practice, the result will work in clangd, and will respect configuration of headers where analysis should be ignored, keep directives, etc)


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:227
+          << Inc.SymRef.Target.name();
+      if (!AlreadyInserted.contains(Inc.Missing.resolvedPath())) {
+        DB << FixItHint::CreateInsertion(
----------------
This means that if you're looking at individual diagnostics rather than applying all fixes to a file, only the first diagnostic will have any fix available at all.

I believe the preferred solution is to do this conditionally based on areDiagsSelfContained(). `clang_tidy::utils::IncludeInserter` encapsulates this, but isn't used here.

I don't know whether we would want to use it here, or how carefully it's already been considered. (It definitely contains a lot of logic that is dubious to apply when the include to insert has already been precisely calculated, but also some things that would be helpful). Will defer to Kadir on that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159263