[PATCH] D40486: [clangd] Implemented logging using Context

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! Just style and comment nits left really.



================
Comment at: clangd/ClangdServer.cpp:216
 
-std::future<Tagged<CompletionList>>
-ClangdServer::codeComplete(PathRef File, Position Pos,
+std::future<std::pair<Tagged<CompletionList>, Context>>
+ClangdServer::codeComplete(Context Ctx, PathRef File, Position Pos,
----------------
this is a pretty gross return type.
Even after (if?) we replace tagged with context, we'll still have completionlist and context.
Should we define a struct here?

(If not, we should consider having the context first, for consistency)


================
Comment at: clangd/ClangdServer.cpp:558
       [this, FileStr, Version,
-       Tag](UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>()>
+       Tag](UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>(
+                const Context &)>
----------------
This wrapping has gone beyond the point of readability for me.
Pull out using `RebuildAction = llvm::Optional<...>(const Context &)`?


================
Comment at: clangd/ClangdServer.h:252
+  ///
+  /// The callers are responsible for making sure \p Ctx stays alive until
+  /// std::future<> is ready (i.e. wait() or get() returns)
----------------
Is this comment obsolete? (and others below)
Ctx is passed by value so I don't understand what it could mean.


================
Comment at: clangd/ClangdServer.h:262
   /// when codeComplete results become available.
-  void codeComplete(UniqueFunction<void(Tagged<CompletionList>)> Callback,
-                    PathRef File, Position Pos,
-                    const clangd::CodeCompleteOptions &Opts,
-                    llvm::Optional<StringRef> OverridenContents = llvm::None,
-                    IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
+  /// The callers are responsible for making sure \p Ctx stays alive until \p
+  /// Callback is executed.
----------------
(and this comment)


================
Comment at: clangd/ClangdServer.h:305
+                                                     llvm::StringRef NewName,
+                                                     const Context &Ctx);
 
----------------
ctx to first param


================
Comment at: clangd/ClangdServer.h:321
 private:
-  std::future<void>
-  scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
-                          std::shared_ptr<CppFile> Resources,
-                          Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
+  std::future<Context> scheduleReparseAndDiags(
+      PathRef File, VersionedDraft Contents, std::shared_ptr<CppFile> Resources,
----------------
ctx to first param


================
Comment at: clangd/ClangdServer.h:325
 
-  std::future<void> scheduleCancelRebuild(std::shared_ptr<CppFile> Resources);
+  std::future<Context> scheduleCancelRebuild(std::shared_ptr<CppFile> Resources,
+                                             Context Ctx);
----------------
ctx to first param


================
Comment at: clangd/ClangdUnit.cpp:555
+                std::shared_ptr<PCHContainerOperations> PCHs) {
+  return std::shared_ptr<CppFile>(new CppFile(
+      FileName, std::move(Command), StorePreamblesInMemory, std::move(PCHs)));
----------------
(while here, this should really be `make_shared`)


================
Comment at: clangd/ClangdUnit.h:175
   llvm::Optional<std::vector<DiagWithFixIts>>
-  rebuild(StringRef NewContents, IntrusiveRefCntPtr<vfs::FileSystem> VFS);
+  rebuild(StringRef NewContents, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+          const Context &Ctx);
----------------
ctx to first param


================
Comment at: clangd/ClangdUnit.h:262
 /// Get definition of symbol at a specified \p Pos.
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+                                      const Context &Ctx);
----------------
ctx to first param


================
Comment at: clangd/CodeComplete.cpp:594
                         std::shared_ptr<PCHContainerOperations> PCHs,
-                        Logger &Logger) {
+                        const Context &Ctx) {
   std::vector<const char *> ArgStrs;
----------------
ctx to first param


================
Comment at: clangd/Function.h:143
+template <class Func> struct ScopeExitGuard {
+  static_assert(std::is_same<typename std::decay<Func>::type, Func>::value,
+                "Func must be decayed");
----------------
FWIW I think this assert adds more noise than value - this can clearly only be triggered by someone instantiating the template directly, which is probably better avoided using `namespace detail` or so.


================
Comment at: clangd/Function.h:157
+
+  ScopeExitGuard(ScopeExitGuard &&Other)
+      : F(std::move(Other.F)), Moved(Other.Moved) {
----------------
I'm struggling to think of cases where moving these guards is the right solution.
Can we delete the move ops until we need them? It seems to make this class almost trivial.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:107
     if (ReturnValue == nullptr)
-      Logger.log("Failed to find compilation database for " + Twine(File) +
-                 "in overriden directory " + CompileCommandsDir.getValue());
+      log(Context::empty(), "Failed to find compilation database for " +
+                                Twine(File) + "in overriden directory " +
----------------
Add a fixme to get a context in here?


================
Comment at: clangd/JSONRPCDispatcher.cpp:145
+
+  auto Ctx = Context::empty()
+                 .derive(RequestOut, &Out)
----------------
There are *lots* of `Context::empty()` calls in this file.
Maybe conceptually clearer for JSONRPCDispatcher to own a "background" context that it derives from? It'd be empty for now in either case, so not a big deal.

The idea being that explicit Context::empty() calls are a bit suspect, but these ones aren't really "different" from each other - we may want to provide an extension point to customize it, but there'd only be one.

Up to you.


================
Comment at: clangd/JSONRPCDispatcher.cpp:148
+                 .derive(RequestSpan, std::move(Tracer));
+  if (ID)
+    Ctx = std::move(Ctx).derive(RequestID, *ID);
----------------
I'm not sure this is better than unconditionally attaching the optional `ID`.
Being able to distinguish between request-with-no-ID and no-request seems like a good thing.


================
Comment at: clangd/JSONRPCDispatcher.h:29
 /// them.
 class JSONOutput : public Logger {
 public:
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > does this class need to be public? Is it staying around for the long haul?
> I'm not sure. I'd rather keep it private, but we don't have the right abstractions to hide it yet, right?
> I'd say this is not directly related to the current patch and should be addressed separately.
It's related in that context takes some of the job of JSONOutput, so now we have a messy overlap.
A FIXME to work out how JSONOutput's role should shrink in light of Context is enough, I think.


================
Comment at: clangd/Protocol.h:24
 
+#include "Context.h"
 #include "JSONExpr.h"
----------------
I think tihs is unused?


================
Comment at: unittests/clangd/ClangdTests.cpp:31
 namespace clangd {
+
 namespace {
----------------
nit: revert?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40486