This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Config: Index.Background
ClosedPublic

Authored by sammccall on Jul 14 2020, 6:23 AM.

Details

Summary

We only support Build/Skip for now, but with 'Load' or similar as an
option for future (load existing shards but don't build new ones).

This requires creating the config for each TU on startup. In LLVM, this
is 4000 occurrences for a total of 800ms on my machine.
But together with caching from D83755 it is only 25ms.

Diff Detail

Event Timeline

sammccall created this revision.Jul 14 2020, 6:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 6:23 AM
kadircet added inline comments.Jul 14 2020, 8:10 AM
clang-tools-extra/clangd/ConfigCompile.cpp
98

should we assert on multiple matches of the same name ?

196

nit: I know this isn't what the patch is about, but.... what about making these lambdas/functions of Out.diag with first parameter bound to DK_Error or DK_Warning? (or having them at an anonymous namespace instead of a member)

clang-tools-extra/clangd/ConfigFragment.h
124

i think saying "outside the current file" might not be the best description. Maybe rather "Controls clangd's understanding for entities(symbols, refs, relations) in the codebase" ?

129

why not store the enum in here directly and generate diagnostics during the parsing stage?

we seem to be doing this for syntactic errors, e.g. expected a dictionary and getting an unexpected enum value sounds similar to that?

sammccall updated this revision to Diff 277844.Jul 14 2020, 8:37 AM
sammccall marked 5 inline comments as done.

Address some comments

clang-tools-extra/clangd/ConfigCompile.cpp
196

I'm not sure about the readability of lambdas here, if we wanted to do this I think the simplest thing would just be to make them methods?

Can we defer this question until after the branch cut?

clang-tools-extra/clangd/ConfigFragment.h
124

I don't really understand this - clangd's understanding for symbols, refs and, relations is all based on the AST, except when we're talking about outside the current file. (And refs and relations aren't terms that make much sense to users)

Expanded this a bit and gave an example.

129

Because then you have to do it twice for JSON vs YAML. You're right this isn't really principled, and I think more data points will help us resolve it.
OK to defer this to after the branch cut?

kadircet accepted this revision.Jul 14 2020, 8:43 AM
kadircet marked an inline comment as done.

let's ship it!

clang-tools-extra/clangd/ConfigCompile.cpp
196

SG 👍

clang-tools-extra/clangd/ConfigFragment.h
129

sure

This revision is now accepted and ready to land.Jul 14 2020, 8:43 AM
This revision was automatically updated to reflect the committed changes.