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

Tue Nov 28 02:16:33 PST 2017

ilya-biryukov added inline comments.

================
Comment at: clangd/Context.cpp:16
+
+static Context *GlobalCtx = nullptr;
+static Context EmptyContext(nullptr, {});
----------------
sammccall wrote:
> 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
Will do

================
Comment at: clangd/Context.h:65
+  Context *Parent;
+  TypedValueMap Data;
+};
----------------
sammccall wrote:
> 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.
Conceptually, a `Context` is more convenient to use when it stores multiple values. This allows to put a bunch of things and assign meaning to `Context` (i.e., a `Context` for processing a single LSP request, global context). If `Context`s were a linked list, the intermediate `Context`s would be hard to assign the meaning to.

That being said, storage strategy for `Context`s is an implementation detail and could be changed anytime. I don't have big preferences here, but I think that storing a linked list of maps has, in general, a better performance than storing a linked list.
And given that it's already there, I'd leave it this way.

================
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
----------------
sammccall wrote:
> 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.
I'd still get rid of it. The less implicit behavior we have, the better.
It does not seem hard to plumb the `Context` all the way through clangd currently. And it should not be too hard in the future.

I was asking myself multiple times whether we needed the global context in the first place while implementing it.
I think we should aim for avoiding global state altogether. That includes singletons, etc. 

================
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.
----------------
sammccall wrote:
> 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)
I think the current interface is simple enough and allows for both storage implementations. I'd really avoid providing an interface that ties us into a single storage implementation.

================
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
----------------
sammccall wrote:
> 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.
Are `Key<>` types confusing? I really like the fact that I don't have to specify type information and it is carried in the `Key<Type>` to the `get`/`emplace` methods, i.e. I don't have to specify template arguments there.
I don't see how `struct RequestId { ID int };` is clearer or shorter than `Key<int> RequestID;`. Again, are `Key<Type>` a confusing concept in the first place?

================
Comment at: clangd/TypedValueMap.h:76
+
+  template <class Type, class Arg> bool emplace(PtrKey<Type> &PtrKey, Arg *A) {
+    return emplace(PtrKey.UnderlyingKey, A);
----------------
sammccall wrote:
> Shouldn't this just be Type *A?
> I think the extra convenience of PtrKey probably isn't worth the complexity, though.
With `Key<A*>` the function will be returning `A**`, which is inconvenient to use.
If we index by `Type*`, as opposed to `Key<Type>`, we'll get the same problems.

For example,
```
Key<Logger*> Key;

Logger ** Val = Map.get(Key); // Or Map.get<Logger*>();
if (!Val)
  return; // No logging is enabled

If (!*Val)
  return; // No logging is enabled, but the nullptr value was found in the map!

(*Val)->logImpl(....); // Log it already!
```

Note an extra if and extra dereference at the last line.
I do think it's a useful addition that outweighs the complexity.

https://reviews.llvm.org/D40485