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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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



================
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))
----------------
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. 
```


================
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());
----------------
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()).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112608