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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly nits - throughout there's going to be lots of judgement calls about where to propagate context and where not.
No need to get all those "exactly right", but trying to get a feel for what these answers are likely to be.

Most of the interesting stuff is around the logger itself of course :-)



================
Comment at: clangd/ClangdServer.cpp:31
 
-class FulfillPromiseGuard {
+class FulfillContextPromiseGuard {
 public:
----------------
Up to you, but I like the old name better - this class is local so it's OK to be a bit opinionated.


================
Comment at: clangd/ClangdServer.cpp:36
 
-  ~FulfillPromiseGuard() { Promise.set_value(); }
+  ~FulfillContextPromiseGuard() { Promise.set_value(std::move(Ctx)); }
 
----------------
Yikes, I can see how we got here, but we really don't get to move out of something we received an lvalue-reference to...

I think a safer idiom here is turning a whole lambda into a destructor:

   auto Fulfil = MakeDestructor([&]{
      DonePromise.set_value(std::move(Ctx));
   });

I'm not sure if LLVM has a shared utility for this, I've seen it in other codebases. Either way we could just define it here.

(Or we could copy the context to avoid the awkwardness)


================
Comment at: clangd/ClangdServer.cpp:220
 
-  // Note that std::future from this cleanup action is ignored.
-  scheduleCancelRebuild(std::move(Recreated.RemovedFile));
+  // Note that std::future from this cleanup action.
+  // FIXME(ibiryukov): We use a global context here, should not fork the action
----------------
You dropped the end of this comment


================
Comment at: clangd/ClangdServer.cpp:221
+  // Note that std::future from this cleanup action.
+  // FIXME(ibiryukov): We use a global context here, should not fork the action
+  // instead.
----------------
I don't quite understand what this is saying should be better. Is it saying we do these two things together instead of running them in parallel?

And why not clone the context instead?


================
Comment at: clangd/ClangdUnit.cpp:436
+      RebuildInProgress(false), PCHs(std::move(PCHs)) {
+  log(emptyCtx(), "Opened file " + FileName + " with command [" +
+                      this->Command.Directory + "] " +
----------------
why are we dropping the context here?
It's important that in general we don't store the ctx in the object and blindly reuse it, but passing it into the constructor for use *in* the constructor seems fine.

Reading on... OK, it saves a *lot* of plumbing to avoid attributing this one API call. I still don't totally understand how the CppFile API/lifetime works, so up to you.


================
Comment at: clangd/ClangdUnit.cpp:533
+                        That](std::string NewContents,
+                              Context &Ctx) mutable // 'mutable' to
+                                                    // allow changing
----------------
nit: this wrapping has become *really* weird over time - can we change the comment to `/* allow changing OldPreamble */` to try to fix it?


================
Comment at: clangd/ClangdUnit.h:257
 
+/// Get signature help at a specified \p Pos in \p FileName.
+SignatureHelp
----------------
I think this is a bad merge - signatureHelp has moved to CodeComplete.h


================
Comment at: clangd/GlobalCompilationDatabase.h:37
   virtual llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const = 0;
 
----------------
Do we want Ctx in this interface?
It's an extension point, and the ability for embedders to track calls flowing between their API calls and their extension points is one of the reasons we have ctx...


================
Comment at: clangd/JSONRPCDispatcher.cpp:22
 
+namespace {
+static Key<std::unique_ptr<trace::Span>> TracerKey;
----------------
Maybe we don't need the Key suffix, as it's in the type... we should spend our verbosity elsewhere


================
Comment at: clangd/JSONRPCDispatcher.cpp:23
+namespace {
+static Key<std::unique_ptr<trace::Span>> TracerKey;
+static Key<json::Expr> IDKey;
----------------
RequestTracer?


================
Comment at: clangd/JSONRPCDispatcher.cpp:24
+static Key<std::unique_ptr<trace::Span>> TracerKey;
+static Key<json::Expr> IDKey;
+static Key<JSONOutput *> OutKey;
----------------
RequestID


================
Comment at: clangd/JSONRPCDispatcher.cpp:48
+void JSONOutput::logImpl(Context &Ctx, const Twine &Message) {
+  // FIXME(ibiryukov): get rid of trace::log here.
   trace::log(Message);
----------------
why?


================
Comment at: clangd/JSONRPCDispatcher.h:29
 /// them.
 class JSONOutput : public Logger {
 public:
----------------
does this class need to be public? Is it staying around for the long haul?


================
Comment at: clangd/JSONRPCDispatcher.h:39
   /// Write a line to the logging stream.
-  void log(const Twine &Message) override;
+  void logImpl(Context &Ctx, const Twine &Message) override;
 
----------------
Is this temporary?


================
Comment at: clangd/Logger.cpp:16
+namespace {
+class EmptyLogger : public Logger {
+public:
----------------
Any need to have an empty logger, rather than just have log() branch?


================
Comment at: clangd/Logger.cpp:22
+EmptyLogger Empty;
+Logger *GlobalLogger = &Empty;
+} // namespace
----------------
There are no non-global loggers. Just L, or Current, or something?


================
Comment at: clangd/Logger.cpp:26
+Logger &globalLogger() {
+  assert(GlobalLogger);
+  return *GlobalLogger;
----------------
I actually think this would be clearer without the assert - this can statically never be null


================
Comment at: clangd/Logger.h:28
   /// Implementations of this method must be thread-safe.
-  virtual void log(const llvm::Twine &Message) = 0;
+  virtual void logImpl(Context &Ctx, const llvm::Twine &Message) = 0;
 };
----------------
why is this not called log()?


================
Comment at: clangd/Logger.h:31
 
-/// Logger implementation that ignores all messages.
-class EmptyLogger : public Logger {
+Logger &globalLogger();
+
----------------
Why does this need to be public?


================
Comment at: clangd/Logger.h:39
 
-  void log(const llvm::Twine &Message) override;
+  LoggingSession(LoggingSession &&) = delete;
+  LoggingSession &operator=(LoggingSession &&) = delete;
----------------
nit: these seem like low SNR. Are we actually worried about people trying to move sessions?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40486