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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!
Most of my comments are of the form "can we make this conceptually simpler, and somewhat less awesome".
This is because it's a somewhat unusual/scary pattern, so I'd like the implementation to be as small and transparent as possible.



================
Comment at: clangd/Context.cpp:16
+
+static Context *GlobalCtx = nullptr;
+static Context EmptyContext(nullptr, {});
----------------
Seems like it would simplify things if:
 - GlobalCtx was always set (static local trick)
 - GlobalSession swapped its context with Global, and then swapped back in its destructor


================
Comment at: clangd/Context.h:64
+private:
+  Context *Parent;
+  TypedValueMap Data;
----------------
nit: const


================
Comment at: clangd/Context.h:65
+  Context *Parent;
+  TypedValueMap Data;
+};
----------------
We add complexity here (implementation and conceptual) to allow multiple properties to be set at the same level (vs having a key and an AnyStorage and making Context a linked list).
Is this for performance? I'm not convinced it'll actually be faster for our workloads, or that it matters.


================
Comment at: clangd/Context.h:71
+/// before any clangd functions are called.
+class GlobalSession {
+public:
----------------
Maybe `WithGlobalContext` is a good name for this scoped object?

This comment doesn't give a clear idea why someone would want to call this.
Maybe `All contexts used by clangd inherit from this global context (including contexts created internally)`


================
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
----------------
ilya-biryukov wrote:
> bkramer wrote:
> > This is a giant code smell. If we want the context route, please pass contexts everywhere. I really don't want this kind of technical debt in clangd now.
> I'm with you on this one, but I think @sammccall was keen on having the global context. The main reason was to always have access to **some** loggers and tracers, even when it's hard to pass the Context into the function.
> It's perfectly easy to remove all usages of `globalCtx()`, currently only 8 usages to get rid of. However, I'd wait for Sam's comment before doing that.
It's important to be able to call functions that require a context if you don't have one - adding a log statement/trace for debugging shouldn't require changing plumbing/interfaces. (It's fine if we want to avoid checking in such code...)
Having an "empty" global context allows this.

At the same time, we want the ability to set the sink for logs/traces etc globally.

A couple of options:
 - the sink (e.g. Logger) is part of the context, we need to allow embedders to set/augment the global context
 - the sink is not stored in the context, instead it is some other singleton the embedder can set up

I don't have a strong opinion which is better. It's nice to reuse mechanisms, on the other hand loggers vs request IDs are pretty different types of data.


================
Comment at: clangd/Context.h:116
+/// Creates a new ContextBuilder, using globalCtx() as a parent.
+ContextBuilder buildCtx();
+/// Creates a new ContextBuilder with explicit \p Parent.
----------------
This seems more naturally a method on Context, e.g.

    Context C = globalCtx().derive(key, value);

The relationship between global and C is clear.

(You can allow chaining+mapping by having Context::derive and ContextBuilder::derive both return ContextBuilder&&, but as noted below I'm not sure it's worth the complexity over Context::derive ->Context)


================
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
----------------
This might be doing a little more than it needs to.

Do we need the ability to have multiple values of the same type?
If request ID is an int, needing to do `struct RequestID { ID int };` doesn't seem like much of a burden, and simplifies the semantics here.

And for cases like Logger where the type carries the semantics, keying by type is clearer.


================
Comment at: clangd/TypedValueMap.h:76
+
+  template <class Type, class Arg> bool emplace(PtrKey<Type> &PtrKey, Arg *A) {
+    return emplace(PtrKey.UnderlyingKey, A);
----------------
Shouldn't this just be Type *A?
I think the extra convenience of PtrKey probably isn't worth the complexity, though.


https://reviews.llvm.org/D40485