[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Context.h:11
+// Context for storing and retrieving implicit data. Useful for passing implicit
+// parameters on a per-request basis.
+//
----------------
sammccall wrote:
> This could use a bit more I think, e.g.
> 
>     A context is an immutable container for per-request data that must be propagated through layers that don't care about it.
>     An example is a request ID that we may want to use when logging.
> 
>     Conceptually, a context is a heterogeneous map<Key<T>, T>. Each key has 
>     an associated value type, which allows the map to be typesafe.
> 
>     You can't add data to an existing context, instead you create a new immutable context derived from it with extra data added. When you retrieve data, the context will walk up the parent chain until the key is found.
> 
>     Contexts should be:
>      - passed by reference when calling synchronous functions
>      - passed by value (move) when calling asynchronous functions. The callback will receive the context again.
>      - copied only when 'forking' an asynchronous computation that we don't wait for
> 
> Some of this is on the class comment - this seems fine but the Context class should then be the first thing in the file.
Done. It's in the class comment, but class is now the first thing in the file.
Thanks for making my life a bit easier and coming up with a doc :-)


================
Comment at: clangd/Context.h:25
+
+class ContextData {
+public:
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > IIUC, the only reason we expose separate `Context` and `ContextData` types is to give things like Span a stable reference to hold onto (`ContextData*`).
> > 
> > Could we use `Context` instead (a copy)? Then we incur `shared_ptr` overhead for span creation, but greatly simplify the interface.
> > If we want to avoid the overhead, the Span could maybe grab whatever it needs out of the context at construction time, for use in the destructor.
> > 
> > Either way, we should make sure `Context` is front-and-center in this header, and I hope we can hide ContextData entirely.
> I have considered making `Context` the only used interface, but I hated the fact that each `Span` constructor would copy a `shared_ptr`.  (And `Span`s can be pretty ubiquitous).
> On the other hand, that should not happen when tracing is off, so we won't be adding overhead when `Span` are not used.
> Will try moving `ContextData` into `.cpp` file, this should certainly be an implementation detail. And we can expose it later if we'll actually have a use-case for that (i.e. not copying `shared_ptr`s).
Done. We now store a copied Context in the Spans.


================
Comment at: clangd/Context.h:32
+  /// specified for \p Key, return null.
+  template <class Type> Type *get(Key<Type> &Key) const {
+    if (auto Val = Data.get(Key))
----------------
sammccall wrote:
> sammccall wrote:
> > nit: const Key&, and below
> this isn't usually how we define const-correctness for containers.
> A const method shouldn't return a mutable reference to the contained object.
Returning const references now.


================
Comment at: clangd/Context.h:42
+  /// Must not be called for keys that are not in the map.
+  template <class Type> Type &getExisting(Key<Type> &Key) const {
+    auto Val = get(Key);
----------------
sammccall wrote:
> I'd prefer the standard name `at` for this function (assert vs throw being pretty minor I think), but YMMV
We've discussed this offline, but just for the record:
I kinda like that `getExisting` is named after `get`. Also `at` usually throws, whereas our function only `asserts` (even though LLVM does not use execptions, we still have undefined behaviour vs crash in runtime).



================
Comment at: clangd/Context.h:139
+/// that require a Context when no explicit Context is not available.
+Context &emptyCtx();
+
----------------
sammccall wrote:
> sammccall wrote:
> > nit: how do you feel about Context::empty()? it ties the name to the type more clearly, I think.
> OOC: why reference and not a value?
> nit: how do you feel about Context::empty()? it ties the name to the type more clearly, I think.
Done.

> OOC: why reference and not a value?
Reference gives us just the right semantics with less overhead (no extra shared_ptr copies) when we need to pass empty context by reference (e.g., to sync functions like `log`).


================
Comment at: clangd/Context.h:142
+/// Creates a ContextBuilder with a null parent.
+ContextBuilder buildCtx();
+
----------------
sammccall wrote:
> I think we should spell this `emptyCtx().derive()`.
> It should be pretty rare to derive from the empty context in production code - and conceptually it's no different than any other use of the empty context, I think.
I'd argue the separate function is more readable and saves us an extra lookup in the empty context for missing keys.
Given that `emptyCtx` is now `Context::empty`, maybe we should also change `buildCtx`  to `Context::build`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40485