This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE320468: [clangd] Introduced a Context that stores implicit
data (authored by ibiryukov, committed by ).
Changed prior t
ilya-biryukov updated this revision to Diff 126515.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.
- Change derive() signature to accept a value instead of the variadic arguments.
- Rewrite a while loop into a for loop.
- Return a value from Context::empty() instead
ilya-biryukov added inline comments.
Comment at: clangd/Context.h:169
+ struct ContextData {
+// We need to make sure Parent outlives the Value, so the order of members
+// is important. We do that to allow classes stored in Context's child
sammccall wro
sammccall accepted this revision.
sammccall added a comment.
Thanks, this looks like exactly the right amount of magic to me :-)
Comment at: clangd/Context.h:91
+ /// functions that require a context when no explicit context is available.
+ static const Context &empty();
+
--
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
Comment at: clangd/Context.h:65
+ Context *Parent;
+ TypedValueMap Data;
+};
klimek wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > ilya-biryu
ilya-biryukov updated this revision to Diff 126373.
ilya-biryukov added a comment.
- Removed ContextBuilder and TypedValueMap.
- Updated the docs.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
unitt
klimek added inline comments.
Comment at: clangd/Context.h:65
+ Context *Parent;
+ TypedValueMap Data;
+};
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > ilya-biryukov wrote:
> > > > sammccall wrote:
> > > > > We add complexity here (impleme
ilya-biryukov updated this revision to Diff 126346.
ilya-biryukov added a comment.
- Removed buildCtx(). Now Contexts can only be created using emptyCtx().derive()
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/
ilya-biryukov added inline comments.
Comment at: clangd/Context.h:92
+ ContextBuilder derive() const &;
+ ContextBuilder derive() const &&;
+
sammccall wrote:
> `&&`, not `const&&` :-)
>
> Maybe add a trailing `//takes ownership`
Right. Copy-paste is gonna kil
ilya-biryukov updated this revision to Diff 126345.
ilya-biryukov added a comment.
- Use `derive() &&` instead of `derive() const &&`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/TypedValue
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:
> sa
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
> > >
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.
>
>
ilya-biryukov updated this revision to Diff 126322.
ilya-biryukov added a comment.
- Replaced emptyCtx with Context::empty
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/TypedValueMap.h
unit
ilya-biryukov updated this revision to Diff 126321.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Added r-value overload for derive().
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
c
ilya-biryukov updated this revision to Diff 126320.
ilya-biryukov added a comment.
- Rephrase the comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/TypedValueMap.h
unittests/clangd/CMa
ilya-biryukov updated this revision to Diff 126319.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.
- Added a comment about the Parent vs Data lifetimes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/
ilya-biryukov updated this revision to Diff 126317.
ilya-biryukov added a comment.
- Return `const Type*` instead of `Type*` in map getters.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/Type
ilya-biryukov updated this revision to Diff 126315.
ilya-biryukov added a comment.
- Made ContextData a private member of Context.
- Added clone method.
- Added forgotten header guard in TypedValueMap.h
- Pass Key<> by const-ref instead of by-ref.
- Pass Context by-const-ref instead of by-ref.
- M
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 (`Con
ioeric added inline comments.
Comment at: clangd/Context.h:63
+/// used as parents for some other Contexts.
+class Context {
+public:
sammccall wrote:
> I think we should strongly consider calling the class Ctx over Context. It's
> going to appear in many functi
sammccall added a comment.
Oops, forgot one important question!
BTW, phabricator seems to have misaligned all my previous comments (you updated
the diff before I submitted them I guess). Let me know if any of them don't
make sense now!
Comment at: clangd/Context.h:63
+/// us
sammccall added a comment.
There's a couple of points where we might be a bit stuck on complexity vs X
tradeoffs (map + contextbuilder, and some of the convenience APIs).
Just want to mention: if I find these things complex, it doesn't mean I think
they're bad code, or even that you should agre
ilya-biryukov updated this revision to Diff 125712.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Removed unused helpers from Context.h
- Use assert instead of llvm_unreachable in getExisiting(Key<>)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.or
ilya-biryukov marked 6 inline comments as done.
ilya-biryukov added a comment.
A second attempt at implementing the `Context`s. Still missing a few comments,
but hopefully ready for review.
Comment at: clangd/Context.h:71
+/// before any clangd functions are called.
+class Glo
ilya-biryukov updated this revision to Diff 125539.
ilya-biryukov added a comment.
Herald added a subscriber: klimek.
Addressed some review comments.
- Removed global context.
- Context now stores a shared_ptr to ContextData to handle lifetimes properly.
- Added helper getters for some commonly u
ilya-biryukov added inline comments.
Comment at: clangd/Context.cpp:16
+
+static Context *GlobalCtx = nullptr;
+static Context EmptyContext(nullptr, {});
sammccall wrote:
> Seems like it would simplify things if:
> - GlobalCtx was always set (static local trick)
sammccall added a comment.
Thanks for doing this!
Most of my comments are of the form "can we make this conceptually simpler, and
somewhat less awesome".
This is because it's a somewhat unusual/scary pattern, so I'd like the
implementation to be as small and transparent as possible.
=
ilya-biryukov added inline comments.
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
bkramer wrote:
> This is a giant code smell. If we want the context route, please pass
> contexts everywhere. I really don't wa
bkramer requested changes to this revision.
bkramer added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
This is a giant code smell. If we want the
ilya-biryukov created this revision.
Herald added a subscriber: mgorny.
It will be used to pass around things like Logger and Tracer throughout
clangd classes.
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/TypedValueMap.h
unitt
31 matches
Mail list logo
