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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Thank you for taking care of this!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:261
+    if (!FE) {
+      assert(isSpecialBuffer(FID, SM));
+      continue;
----------------
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.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:261
+    if (!FE) {
+      assert(isSpecialBuffer(FID, SM));
+      continue;
----------------
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"


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:275
+        SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // (Note we deduped the names as _number_ of <built-in>s is uninteresting).
+  EXPECT_THAT(ReferencedFileNames.keys(),
----------------
nit: Drop the parenthesis?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112652