Page MenuHomePhabricator

[clangd] Change prepareCompilerInstance to take an FSProvider
Needs ReviewPublic

Authored by kadircet on Jun 12 2020, 1:31 AM.

Details

Reviewers
sammccall
Summary

This makes API easier to use by moving setcwd and fscache setup into it.
Also ensures no side effects are observable on the VFS used by the
compiler instance.

Diff Detail

Event Timeline

kadircet created this revision.Jun 12 2020, 1:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2020, 1:31 AM
kadircet updated this revision to Diff 270333.Jun 12 2020, 2:12 AM
  • Use Cmd.Directory when creating includefixer.

Yay for getting rid of more VFSes passed around. Not sure about some of the other signature changes to prepareCompilerInstance. It seems a bit harder to call now.

In particular I'm not sure whether moving the preamble-stat-cache across setup from caller to callee is really an improvement once we modify that to wrap a ThreadsafeFS instead of a FS.
It's just... a reminder to use the cache, I guess? But the , nullptr, nullptr is annoying where we don't care about that.

clang-tools-extra/clangd/Compiler.cpp
104

why is this a reference if it can't be null?

(otherwise, remove the assert message - just repeats the assertion)

clang-tools-extra/clangd/Compiler.h
78

none of the new parameter names say anything beyond repeating the types, can we drop them?

79

nit: grouping of parameters seems a bit confusing, preamble + mainfile together (in either order) was clearer I think. DiagnosticConsumer is logically an output param, nice to keep it at the end. Why split the FSProvider and the FS cache, etc.

80

It doesn't seem reasonable to require both CompilerInvocation and CompileCommand. We can pass working dir in if we must...

(CompilerInvocation actually has a working directory field, but it looks like it potentially changes behavior. We could consider setting it in buildCompilerInvocation() in future...)

clang-tools-extra/clangd/ParsedAST.cpp
356

you appear to have added the fixme and... also fixed it

clang-tools-extra/clangd/index/Background.cpp
273

hmm!

clang-tools-extra/clangd/unittests/PreambleTests.cpp
78

why this change?