[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Context.h:163
+
+  template <class Type> Context derive(Type &&Value) const && {
+    static Key<typename std::decay<Type>::type> Private;
----------------
s/`const &&`/`&&`


================
Comment at: clangd/Context.h:165
+    static Key<typename std::decay<Type>::type> Private;
+    return derive(Private, std::forward<Type>(Value));
+  }
----------------
This will call the `const&` version of `derive(Key, Value)`.
To move we need to do `std::move(*this).derive(Private, std::forward<Type>(Value))`


================
Comment at: clangd/Context.h:172
+  friend bool operator==(const Context &A, const Context &B) {
+    return A.DataPtr == B.DataPtr;
+  }
----------------
I wonder what are the use-cases for this `operator ==`?


================
Comment at: clangd/Function.h:147
 
-  ScopeExitGuard(Func F) : F(std::move(F)) {}
+  ScopeExitGuard(Func F) : F(llvm::make_unique<Func>(std::move(F))) {}
   ~ScopeExitGuard() {
----------------
Why do we need `unique_ptr` instead of `Optional` here?


================
Comment at: clangd/Trace.h:92
+/// Ctx (or some ancestor) must be created by span().
+/// This macro is not safe for use on the same span from multiple threads.
+#define SPAN_ATTACH(Ctx, Name, Expr)                                           \
----------------
This is too easy to misuse unintentionally. Will this go away if we move to thread-local contexts? I.e. will the API leave out mutations of the context that can be used concurrently


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42499