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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 17 09:02:57 PST 2018


sammccall added a comment.

This looks better overall to me.

I'm not sure about clangdserver rather than clangdunit computing the commands. You're probably right but I'd like you to explain it to me :-)
Rest is just readability nits.



================
Comment at: clangd/ClangdServer.cpp:35
 
+tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB,
+                                          PathRef File, PathRef ResourceDir) {
----------------
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?


================
Comment at: clangd/ClangdServer.cpp:336
+  assert(Resources->getLatestCommand() &&
+         "CppFile is in inconsistent state, missing CompileCommand");
+  tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
----------------
what's inconsistent about this state? (and above)


================
Comment at: clangd/ClangdServer.cpp:337
+         "CppFile is in inconsistent state, missing CompileCommand");
+  tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
+
----------------
this seems better off inlined?


================
Comment at: clangd/ClangdServer.cpp:577
+
+  llvm::Optional<tooling::CompileCommand> FileCmd =
+      Resources->getLatestCommand();
----------------
FWIW this might be easier to parse, and avoids dead copies, as

  Optional<CompileCommand> ReusedCommand;
  if (AllowCachedFlags)
    ReusedCommand = Resources->getLatestCommand();
  CompileCommand Cmd = ReusedCommand ? std::move(*ReusedCommand) : getCompileCommand();
  ParseInput Inputs(std::move(Cmd), ...)


================
Comment at: clangd/ClangdServer.h:335
+                          Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS,
+                          bool UpdateCompileCommand);
 
----------------
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)


================
Comment at: clangd/ClangdUnit.cpp:485
+    auto &VFS = Inputs.FS;
+    auto &NewContents = Inputs.Contents;
+
----------------
nit: alias doesn't seem needed


================
Comment at: clangd/ClangdUnit.h:62
+/// Information required to run clang (e.g., to parse AST or do code
+/// completion).
+struct ParseInputs {
----------------
unwrap?


================
Comment at: clangd/ClangdUnit.h:67
+
+  /// Compilation arguments.
+  tooling::CompileCommand CompileCommand;
----------------
these comments just echo the type/name, remove?


================
Comment at: clangd/ClangdUnit.h:192
   /// diagnostics were produced.
-  llvm::Optional<std::vector<DiagWithFixIts>>
-  rebuild(const Context &Ctx, StringRef NewContents,
-          IntrusiveRefCntPtr<vfs::FileSystem> VFS);
+  llvm::Optional<std::vector<DiagWithFixIts>> rebuild(const Context &Ctx,
+                                                      ParseInputs Inputs);
----------------
should ParseInputs be &&? It's big, if callers are copying it something went wrong.


================
Comment at: clangd/ClangdUnit.h:229
+  /// cancelRebuild().
+  llvm::Optional<tooling::CompileCommand> getLatestCommand() const;
 
----------------
nit: consider `getLastCommand` which removes "latest"'s ambiguity between "previous command used" and "current command, bypassing any caches"


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42173





More information about the cfe-commits mailing list