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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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
----------------
sammccall wrote:
> You dropped the end of this comment
Missed that. Thanks.


================
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.
----------------
sammccall wrote:
> 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?
The first implementation did not allow to copy the `Context`, hence the comment. I've removed it and the context is now copied.


================
Comment at: clangd/ClangdUnit.cpp:436
+      RebuildInProgress(false), PCHs(std::move(PCHs)) {
+  log(emptyCtx(), "Opened file " + FileName + " with command [" +
+                      this->Command.Directory + "] " +
----------------
sammccall wrote:
> 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.
Exactly, plumbing it here is hard, so we're dropping the context here for now. Most of the useful work is still done outside the constructor, so this seems fine.
The threading patch will make it easier to pass proper Context here. I've added a FIXME.


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


================
Comment at: clangd/GlobalCompilationDatabase.h:37
   virtual llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const = 0;
 
----------------
sammccall wrote:
> 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...
I think we do, but I'd add this in a separate patch. (We're currently not changing the behavior, simply plumbing the Context through all the APIs).
Happy to come up with a follow-up patch, though.


================
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);
----------------
sammccall wrote:
> why?
This was a reminder to myself to discuss whether we want it globally in tracing+logging, not only in `JSONOutput`.
Removed it from the code.


================
Comment at: clangd/JSONRPCDispatcher.cpp:70
+
+  SPAN_ATTACH(*Ctx->getExisting(TracerKey), "Reply", Result);
+  Ctx->getPtr(OutKey)->writeMessage(json::obj{
----------------
sammccall wrote:
> Depending on a span tracer existing seems needlessly brittle. What about
> 
>   if (auto *Tracer = Ctx->get(TracerKey))
>     SPAN_ATTACH(*Tracer, ...)
Done.
Turned out to be `SPAN(**Tracer, ...`. (Note the double dereference).


================
Comment at: clangd/JSONRPCDispatcher.h:29
 /// them.
 class JSONOutput : public Logger {
 public:
----------------
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.


================
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;
 
----------------
sammccall wrote:
> Is this temporary?
Renamed back to `log`.
It's a leftover from an older version of the patch.


================
Comment at: clangd/Logger.cpp:22
+EmptyLogger Empty;
+Logger *GlobalLogger = &Empty;
+} // namespace
----------------
sammccall wrote:
> There are no non-global loggers. Just L, or Current, or something?
This is really just a preference. To me the global prefix is not about distinguishing it from other things in this file, but rather about signalling what is this thing so that I don't have to read the declaration before reading the usages to understand what's going on. Having `L` or `Current` requires to be in the context of the file.

Updated the code to use `L` instead.


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


================
Comment at: clangd/Logger.h:39
 
-  void log(const llvm::Twine &Message) override;
+  LoggingSession(LoggingSession &&) = delete;
+  LoggingSession &operator=(LoggingSession &&) = delete;
----------------
sammccall wrote:
> nit: these seem like low SNR. Are we actually worried about people trying to move sessions?
If they do, that will lead to a bug (destructor being called twice), so it feels safer to delete the corresponding constructors.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40486