klimek added inline comments.
================
Comment at: clangd/Context.h:65
+ Context *Parent;
+ TypedValueMap Data;
+};
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > ilya-biryukov wrote:
> > > > 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.
> > > With the new shared_ptr semantics:
> > >
> > > Context D = move(C).derive(K1, V1).derive(K2, V2);
> > >
> > > Is just as meaningful as
> > >
> > > Context D = move(C).derive().add(K1, V1).add(K2, V2);
> > >
> > > Yeah, the list of maps in an implementation detail. It's one that comes
> > > with a bunch of complexity (`ContextBuilder` and most of
> > > `TypedValueMap`). It really doesn't seem to buy us anything (the
> > > performance is both uninteresting and seems likely to be worse in this
> > > specific case with very few entries).
> > The thing I like about it is that the `Context`s are layered properly in a
> > sense that there's a Context corresponding to the request, a Context
> > corresponding to the forked subrequests, etc.
> > If we change the interface, we'll be creating a bunch of temporary Contexts
> > that don't correspond to a nice meaningful abstraction (like request) in my
> > head, even though we don't give those contexts any names.
> >
> > I do agree we currently pay with some complexity for that. Though I'd argue
> > it's all hidden from the users of the interface, as building and consuming
> > contexts is still super-easy and you don't need to mention ContextBuilder
> > or TypedValueMap. And the implementation complexity is totally manageable
> > from my point of view, but I am the one who implemented it in the first
> > place, so there's certainly a bias there.
> I don't see temporary unnamed `Context`s being any different from temporary
> unnamed `ContextBuilder`s.
>
> But we've gone around on this point a bit, and this really seems to be a
> question of taste. @klimek, can we have a third opinion?
>
> The options we're looking at are:
> - `Context` stores a map and a parent pointer. `derive()` returns a
> `ContextBuilder` used to create new contexts containing 0 or more new KV
> pairs. `TypedValueMap` stores the payloads.
> - `Context` stores a single KV pair and a parent pointer. `derive(K, V)` is
> used to create a new context with one new KV pair. A Key-pointer and
> AnyStorage in `Context` store the payloads, the rest of `TypedValueMap` goes
> away.
I'd agree that Context::derive(K, V) would be simpler here, mainly because
there is only one way to pass around contexts while we're building them up;
specifically, there's no temptation to pass around ContextBuilders.
================
Comment at: clangd/TypedValueMap.h:38
+
+/// A type-safe map from Key<T> to T.
+class TypedValueMap {
----------------
Should we say "from Key<T>*" or &? If you say Key<T> I expect something that
compares by value.
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
