[PATCH] D112695: [clangd] IncludeCleaner: Skip non self-contained headers

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
   for (const Inclusion &MFI : Structure.MainFileIncludes) {
     // FIXME: Skip includes that are not self-contained.
     if (!MFI.HeaderID) {
----------------
sammccall wrote:
> this is fixed now (though I think it would be better addressed ~here)
That's what I had initially (you can see the previous version of the patch, after which I sent "oh, wrong place" and changed it). I think my idea was that this looks confusing from the API standpoint. Choosing to filter out non-guarded headers looks like a diagnostics decision and it looked awkward to me to do that here: what `getUnused` does is simply merge two data structures. I think it's somewhat similar to the patch where you mentioned "parse, don't validate" and it is a great argument: the `<built-in>` and `<scratch-space>` are indeed not "headers" in a sensible meaning of the term but non self-contained ones are.

I don't have a very strong opinion on this, so I put the filtering back here but I'm curious if you agree with my thoughs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112695