[PATCH] D42173: [clangd] Simplify code handling compile commands

[Ilya Biryukov via Phabricator via cfe-commits]

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