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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
