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