sammccall added a comment. Thanks for the changes. I don't think `TypedValueMap`/`ContextBuilder` pull their weight, but let's get another opinion on this.
================ Comment at: clangd/Context.h:65 + Context *Parent; + TypedValueMap Data; +}; ---------------- 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. ================ Comment at: clangd/Context.h:142 +/// Creates a ContextBuilder with a null parent. +ContextBuilder buildCtx(); + ---------------- ilya-biryukov wrote: > sammccall wrote: > > I think we should spell this `emptyCtx().derive()`. > > It should be pretty rare to derive from the empty context in production > > code - and conceptually it's no different than any other use of the empty > > context, I think. > I'd argue the separate function is more readable and saves us an extra lookup > in the empty context for missing keys. > Given that `emptyCtx` is now `Context::empty`, maybe we should also change > `buildCtx` to `Context::build`? I think this isn't a path we want to make smooth or obvious - usually what you want is to accept a context, derive from it, and use the result. Embedders will need to create root contexts - we discussed `LSPServer` offline, and there it seems natural to own a background context and derive from it per handled call. ================ Comment at: clangd/Context.h:92 + ContextBuilder derive() const &; + ContextBuilder derive() const &&; + ---------------- `&&`, not `const&&` :-) Maybe add a trailing `//takes ownership` 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
