ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdServer.cpp:35
+tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB,
+ PathRef File, PathRef ResourceDir) {
----------------
sammccall wrote:
> This seems like something of an odd fit: the clangdserver both produces and
> consumes the compile commands, but ClangdUnit is responsible for storing it?
>
> Are we sure it wouldn't be clearer to keep the "get compile command" action
> in clangd unit (and propagate the "should rescan" flag), and just encapsulate
> the resource-dir/fallback logic a bit better?
I've put the command into `ClangdUnit` to not change the code consuming compile
commands (this is where you currently get compile commands from).
The final goal is to remove compile command from `ClangdUnit` and store it
beside the contents of the file (somewhere inside ClangdServer or its component
that manages the resources), ClangdUnit will only manage the built
ASTs/Preambles.
This is what `ParseInputs` is for, it captures all things necessary to build
AST/Preamble. When we'll start dropping ASTs for non-accessed files, we could
be storing ParseInputs instead to be able to recreate the ASTs when necessary.
================
Comment at: clangd/ClangdServer.cpp:336
+ assert(Resources->getLatestCommand() &&
+ "CppFile is in inconsistent state, missing CompileCommand");
+ tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
----------------
sammccall wrote:
> what's inconsistent about this state? (and above)
There's an invariant that if a file is tracked, its compile command in CppFile
should be set.
E.g., `!!(Untis.getFile(File)) ==
!!(Untis.getFile(File))->getLatestCompileCommand`.
We should probably spell it out explicitly in the assertion message. E.g.
`"getFile() must only return files with populated commands"`
WDYT?
================
Comment at: clangd/ClangdServer.h:335
+ Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS,
+ bool UpdateCompileCommand);
----------------
sammccall wrote:
> The name `UpdateCompileCommand` is confusing in the case that this file
> hasn't been seen: it's not obvious whether you have to pass true, or whether
> it doesn't matter.
>
> Consider `AllowCachedFlags`, and inverting the sense?
> At least for me, it's more obvious that this flag is ignored if the cache is
> empty.
> (or AllowCachedCompileCommand, which is a bit long for my taste, or
> AllowCachedCommand, which is a bit vague)
I like `CachedFlags`, but it also seems a bit vague in the same sense that
`CachedCommand` is vague.
I've opted for `AllowCachedCompileFlags`, it's long but shouldn't cause any
confusion.
================
Comment at: clangd/ClangdUnit.h:67
+
+ /// Compilation arguments.
+ tooling::CompileCommand CompileCommand;
----------------
sammccall wrote:
> these comments just echo the type/name, remove?
Sorry, I thought I removed them prior to doing the commit.
Thanks for spotting that.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
