[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

[Ilya Biryukov via Phabricator via cfe-commits]

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