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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added a comment.

Thanks!



================
Comment at: clangd/Context.h:77
+  /// See WithContext and WithContextValue for how to do this safely.
+  static Context &current();
 
----------------
ilya-biryukov wrote:
> 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.
Done (named `swap`).

In fact the only caller is `WithContext`, which is a good sign.

It would be possible to make `swap()` private, since WithContext is in this file. I tend to lean against it because embedders will do explicit context manipulation and WithContext may not be the right primitive for them. So I've left the WithContext guidance as "just a comment". Could be convinced though.



================
Comment at: clangd/Context.h:201
+  WithContext &operator=(const WithContext &) = delete;
+  WithContext(WithContext &&) = default; // Allow return-by-value.
+  WithContext &operator=(WithContext &&) = delete;
----------------
ilya-biryukov wrote:
> Where do we use it? Can't we make move ctor deleted too?
Good catch! This was used in an old version of the patch.


================
Comment at: clangd/Trace.cpp:131
+    return Context::current().clone();
+  WithContextValue WithArgs{std::unique_ptr<json::obj>(Args)};
+  return T->beginSpan(Name, Args);
----------------
ilya-biryukov wrote:
> Maybe use parentheses instead of braces for initialization?
Braces here are to avoid most-vexing-parse: with parens this is a function declaration.

(I'm not sure this is worth a comment, as the compiler flags it if you change it)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42517