[PATCH] D42517: [clangd] Pass Context implicitly using TLS.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Context.h:77
+  /// See WithContext and WithContextValue for how to do this safely.
+  static Context &current();
 
----------------
Maybe we could return `const Context&` here and have a separate `setCurrent()` method for replacing the current context?
Most of the clients shouldn't mutate the contexts anyway, and finding the few that do seems easier if there's a separate method for that.


================
Comment at: clangd/Context.h:201
+  WithContext &operator=(const WithContext &) = delete;
+  WithContext(WithContext &&) = default; // Allow return-by-value.
+  WithContext &operator=(WithContext &&) = delete;
----------------
Where do we use it? Can't we make move ctor deleted too?


================
Comment at: clangd/JSONRPCDispatcher.h:60
+/// Current context must derive from JSONRPCDispatcher::Handler.
+void reply(json::Expr &&Result);
+/// Sends an error response to the client, and logs it.
----------------
`reply`'s signature is scary now.
Should we change it to accept something request-specific that can be found in the `Context` by the clients? WDYT? 

PS we could change it after the patch lands, just wanted to get your opinion on this


================
Comment at: clangd/Threading.cpp:37
           // addToFront).
-          Request = std::move(RequestQueue.front());
+          Request = std::move(RequestQueue.front().first);
+          Ctx = std::move(RequestQueue.front().second);
----------------
Maybe replace with `std::tie(Request, Ctx) = std::move(RequestQueue.front())`?


================
Comment at: clangd/Trace.cpp:131
+    return Context::current().clone();
+  WithContextValue WithArgs{std::unique_ptr<json::obj>(Args)};
+  return T->beginSpan(Name, Args);
----------------
Maybe use parentheses instead of braces for initialization?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42517