ilya-biryukov added inline comments.
================
Comment at: clangd/Context.h:25
+
+class ContextData {
+public:
----------------
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).
================
Comment at: clangd/Context.h:63
+/// used as parents for some other Contexts.
+class Context {
+public:
----------------
ioeric wrote:
> sammccall wrote:
> > I think we should strongly consider calling the class Ctx over Context.
> > It's going to appear in many function signatures, and I'm not sure the
> > extra characters buy us anything considering the abbreviation is pretty
> > unambiguous, and the full version isn't very explicit.
> I have no opinion on the names... Just wondering: if the class is called
> `Ctx`, what name do you suggest to use for variables? With `Context`, you can
> probably use `Ctx`. But... LLVM variable naming is sad... :(
I'd also go with `Context Ctx` rather than `Ctx C`. I'd say `Context` is quite
a small and concise name. It even reads better than `Ctx` to me.
But I'm fine either way, so will change the name to `Ctx`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
