ilya-biryukov added inline comments.
================ Comment at: clangd/Context.h:65 + Context *Parent; + TypedValueMap Data; +}; ---------------- 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. 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
