[PATCH] D112608: [clangd] IncludeCleaner: Do not process locations in built-in files

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In D112608#3090910 <https://reviews.llvm.org/D112608#3090910>, @sammccall wrote:

> Sorry to be late to the party, just some efficiency concerns

No problem, thank you for the comments, I'll send a follow-up!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:133
+    // Check if Loc is not written in a physical file.
+    if (FID.isInvalid() || SM.isWrittenInBuiltinFile(Loc) ||
+        SM.isWrittenInCommandLineFile(Loc))
----------------
sammccall wrote:
> This part of the function is a hot path: we haven't deduplicated FileIDs yet. And isWrittenInBulitinFile/CommandLineFile are not cheap (seriously, go look at the implementation of getPresumedLoc. I'm not sure *why* we need to handle the 'presumed' cases there, either).
> 
> I think the simple/good thing is to allow this to hit Files.insert(FID) and then filter those out later instead.
> 
> This could be in a simple walk over the DenseSet at the end, but translateToHeaderIDs is actually looking at the FileEntries for each header anyway. My guess is we're crashing in this code:
> 
> ```
>     const FileEntry *FE = SM.getFileEntryForID(FID);
>     assert(FE); // this assert passes, we get a fake FE for "<built in>", "<command line>" or "<scratch>"
>     // Option 1: we could bail out here with a simple check on FE->getName().
>     const auto File = Includes.getID(FE);
>     assert(File); // this assert fails. Option 2: turn this assert into an if instead. 
> ```
Hmm, good idea, I'll resolve this in a follow-up!

P.S. Yes, we're crashing on `assert(FE)` in here.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:148
     // ScratchBuffer). That is not a real file we can include.
     if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
       add(Exp.getSpellingLoc());
----------------
sammccall wrote:
> FWIW, the same seems to apply here (though at least we've deduplicated file IDs).
> 
> I don't think we need to do the expensive check to avoid adding scratch buffers to the list, when we can just filter them out at the end with a cheaper check.
> (In any case I don't think it makes much sense to check for scratch *before* calling add(), but check for builtin/cli *inside* add()).
Yeah, I was trying to figure out if this would apply to the macro expansions/spellings but couldn't figure out a reasonable example, so I left it out. I agree that it should probably be handled, too, I just wasn't sure when this could happen.

If we do post-filtering, I guess we can handle all of the locations at once, so probably safe to even remove these checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112608