kadircet marked 3 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:80
+ std::string Driver;
+ std::string Directory;
+ // Whether certain includes should be part of query.
----------------
nridge wrote:
> It looks like the purpose of including `Directory` in the key is to use it in
> `make_absolute` in case where `Driver` is not an absolute path.
>
> I wonder if it would make sense to perform that `make_absolute` operation
> eagerly and store its result in the key instead.
>
> I'm thinking of a scenario where a project has say 10,000 source files in 500
> directories, and in the `compile_commands.json` entries the `directory` is
> the directory immediately containing the source file (or a corresponding
> directory in a `build/` directory hierarchy). In such cases, having the
> directory be in the key would mean executing the driver 500 times instead of
> just once (assuming no other properties are different) during indexing.
yeah makes sense. i was mostly trying to avoid the extra string operations,
with the assumption of "directory" being ~always the same across entries. but
we're already paying for those string operations today, so not saving them
won't be a regression and being more resilient for such cases in the future
wouldn't hurt.
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:138
+ if (Type == driver::types::TY_INVALID) {
+ elog("System include extraction: invalid file type for {0}", Ext);
+ } else {
----------------
nridge wrote:
> The previous behaviour was to abort system include extraction in this case.
>
> Assuming you're not looking to change that behaviour, one way to preserve it
> could be to have `render()` check for an empty `Lang` and return an empty
> vector in that case, and also have `extractSystemIncludesAndTarget()` check
> for `render()` returning an empty vector and return an empty optional in that
> case.
thanks! definitely didn't notice that, I am not sure how valid of a concern
this is in practice. as we'll only use the cached result for translation units
whose language cannot be determined (I'd claim they all belong to same
category). but leaving it as-is in this patch with a fixme (happy to drop the
fixme if you think that's actually important). made it similar to existing code
pattern, i.e. bailing out early if language is empty.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146941/new/
https://reviews.llvm.org/D146941
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
