[PATCH] D112608: [clangd] IncludeCleaner: Do not process locations in built-in files
Kirill Bobyrev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 27 11:26:12 PDT 2021
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
More information about the cfe-commits
mailing list