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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 6 inline comments as done.
ilya-biryukov added a comment.

A second attempt at implementing the `Context`s. Still missing a few comments, but hopefully ready for review.



================
Comment at: clangd/Context.h:71
+/// before any clangd functions are called.
+class GlobalSession {
+public:
----------------
sammccall wrote:
> 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)`
Remove the global context altogether.


================
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
----------------
ilya-biryukov wrote:
> 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. 
Removed the global context.


================
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.
----------------
ilya-biryukov wrote:
> 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.
Added a `derive()` method to the `Context`.  It currently returns `ContextBuilder`, I still think it's nice to have the `ContextBuilder` and a separation between an immutable `Context` and a mutable `ContextBuilder`. Happy to chat on whether we want to remove it.


================
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
----------------
ilya-biryukov wrote:
> 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?
Unfortunately, `typeid` does not with `-fno-rtti`, so there does not seem to be a way to significantly simplify the implementation (i.e. we'll need some form of `Key<Type>` anyway, at least internally).
Therefore I'd keep the current interface. Does that sound ok?


================
Comment at: clangd/TypedValueMap.h:76
+
+  template <class Type, class Arg> bool emplace(PtrKey<Type> &PtrKey, Arg *A) {
+    return emplace(PtrKey.UnderlyingKey, A);
----------------
ilya-biryukov wrote:
> 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.
Removed `PtrKey`, added a `getPtr` helper method to the `ContextData` instead (that does the same thing).
It's useful to have those easy-to-use helpers for common cases.

Totally agree that having a separate `PtrKey` for them is an overkill.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40485