ilya-biryukov added inline comments.
================
Comment at: clangd/Context.h:11
+// Context for storing and retrieving implicit data. Useful for passing
implicit
+// parameters on a per-request basis.
+//
----------------
sammccall wrote:
> This could use a bit more I think, e.g.
>
> A context is an immutable container for per-request data that must be
> propagated through layers that don't care about it.
> An example is a request ID that we may want to use when logging.
>
> Conceptually, a context is a heterogeneous map<Key<T>, T>. Each key has
> an associated value type, which allows the map to be typesafe.
>
> You can't add data to an existing context, instead you create a new
> immutable context derived from it with extra data added. When you retrieve
> data, the context will walk up the parent chain until the key is found.
>
> Contexts should be:
> - passed by reference when calling synchronous functions
> - passed by value (move) when calling asynchronous functions. The
> callback will receive the context again.
> - copied only when 'forking' an asynchronous computation that we don't
> wait for
>
> Some of this is on the class comment - this seems fine but the Context class
> should then be the first thing in the file.
Done. It's in the class comment, but class is now the first thing in the file.
Thanks for making my life a bit easier and coming up with a doc :-)
================
Comment at: clangd/Context.h:25
+
+class ContextData {
+public:
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > IIUC, the only reason we expose separate `Context` and `ContextData` types
> > is to give things like Span a stable reference to hold onto
> > (`ContextData*`).
> >
> > Could we use `Context` instead (a copy)? Then we incur `shared_ptr`
> > overhead for span creation, but greatly simplify the interface.
> > If we want to avoid the overhead, the Span could maybe grab whatever it
> > needs out of the context at construction time, for use in the destructor.
> >
> > Either way, we should make sure `Context` is front-and-center in this
> > header, and I hope we can hide ContextData entirely.
> I have considered making `Context` the only used interface, but I hated the
> fact that each `Span` constructor would copy a `shared_ptr`. (And `Span`s
> can be pretty ubiquitous).
> On the other hand, that should not happen when tracing is off, so we won't be
> adding overhead when `Span` are not used.
> Will try moving `ContextData` into `.cpp` file, this should certainly be an
> implementation detail. And we can expose it later if we'll actually have a
> use-case for that (i.e. not copying `shared_ptr`s).
Done. We now store a copied Context in the Spans.
================
Comment at: clangd/Context.h:32
+ /// specified for \p Key, return null.
+ template <class Type> Type *get(Key<Type> &Key) const {
+ if (auto Val = Data.get(Key))
----------------
sammccall wrote:
> sammccall wrote:
> > nit: const Key&, and below
> this isn't usually how we define const-correctness for containers.
> A const method shouldn't return a mutable reference to the contained object.
Returning const references now.
================
Comment at: clangd/Context.h:42
+ /// Must not be called for keys that are not in the map.
+ template <class Type> Type &getExisting(Key<Type> &Key) const {
+ auto Val = get(Key);
----------------
sammccall wrote:
> I'd prefer the standard name `at` for this function (assert vs throw being
> pretty minor I think), but YMMV
We've discussed this offline, but just for the record:
I kinda like that `getExisting` is named after `get`. Also `at` usually throws,
whereas our function only `asserts` (even though LLVM does not use execptions,
we still have undefined behaviour vs crash in runtime).
================
Comment at: clangd/Context.h:139
+/// that require a Context when no explicit Context is not available.
+Context &emptyCtx();
+
----------------
sammccall wrote:
> sammccall wrote:
> > nit: how do you feel about Context::empty()? it ties the name to the type
> > more clearly, I think.
> OOC: why reference and not a value?
> nit: how do you feel about Context::empty()? it ties the name to the type
> more clearly, I think.
Done.
> OOC: why reference and not a value?
Reference gives us just the right semantics with less overhead (no extra
shared_ptr copies) when we need to pass empty context by reference (e.g., to
sync functions like `log`).
================
Comment at: clangd/Context.h:142
+/// Creates a ContextBuilder with a null parent.
+ContextBuilder buildCtx();
+
----------------
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`?
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
