[PATCH] D112652: [clangd] Avoid expensive checks of buffer names in IncludeCleaner

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:261
+    if (!FE) {
+      assert(isSpecialBuffer(FID, SM));
+      continue;
----------------
kbobyrev wrote:
> kbobyrev wrote:
> > Implementation-wise, this seems like the right place to drop nonexistent "headers". However, this is a very surprising behavior from something like "translateToHeaderIDs" for two reasons:
> > 
> > - As the name states, it used to be a simple "translation" process, now it's also filtering.
> > - `TranslatedHeaderIDs.size() == Files.size()` can be rather unexpected
> > 
> > We never had 1:1 order etc translation assumptions but this is certainly the new behavior we want to document.
> Thinking out loud: now I'm also thinking about the "handle non self-contained files" + "drop warnings for files without include warnings"
This function is mapping from a domain (FileIDs excluding macros) where special buffers exist to a codomain (HeaderIDs) where they don't. Some sort of filtering/handling is implied by the signature!
So I think it's not surprising at least if you think about it that way :-) But this definitely needs to be documented, I'll do that.

> Thinking out loud: now I'm also thinking about the "handle non self-contained files" + "drop warnings for files without include warnings"

There may some small efficiency from mashing those into this loop as well, but I don't think it's worth the confusion.
The reason I put this one inline instead of as a separate filtering step is that the check we'd be doing is a natural part of the conversion. It's basically a form of [parse, don't validate](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112652