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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
