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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
