[PATCH] D113262: [clangd] Allow IncludeCleaner to replace unused header with transitively used

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Oh, sorry, I marked it as "changes intended" so that you could know it's not review-ready yet :) I was just playing around with it and previewing to see how it would feel to have a somewhat working solution.

I didn't really implement the feature (add the proper header spelling that we have in `IncludeFixer` as you mentioned in the comments, check if the replacement header is already included or not, etc), but there are two important problems I was facing that look rather tricky

- This actually steps into the "missing includes" land: we can be conservative when deciding whether a header is used or not. It is both system headers _and_ (currently) a lot of noise in the form of headers with forward decls. E.g. for StringRef the replacement set includes `SHA1` and a bunch of very surprising headers just because we have lots of forward decls in unexpected places. I originally had the idea that we can do unused includes without missing includes but it seems that they are closer in terms of at least this usecase. I wanted to see if we would need something more but maybe the follow-up on the mentioned patch (narrow down the "used" header property set conditions) would be enough.
- Performance: providing fix-its for individual headers is not really performance-optimal: we need a graph traversal from each MainFileInclusion and it's a linear BFS, so the complexity is O(unused headers * graph size). This _might_ be unfortunate for large graphs and (surprisingly) it's just faster to collect the data for a "batch replace" of unused headers with used ones. But this brings us back into the realm of missing headers. Maybe the performance is fine, but I want to estimate it in a better way. So far I've seen somewhat small graphs when editing LLVM (<1k headers) but it might be different for different parts so I want to check the performance to make sure it's really fine.

Anyway, I'd be happy to talk once I'm back but as you mentioned this patch would probably need some QOL improvements before it can be landed (needless to say, it should be complete, too :) )!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113262