[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 to commit:
https://reviews.llvm.org/D40485?vs=126515&id=126518#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- unittests/clangd/ContextTests.cpp
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,57 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(ContextTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ Context Ctx = Context::empty().derive(IntParam, 10).derive(ExtraIntParam, 20);
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(ContextTests, MoveOps) {
+ Key> Param;
+
+ Context Ctx = Context::empty().derive(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ Context NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ Context::empty().derive(ParentParam, 10).derive(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive(ParentAndChildParam, 30).derive(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -8,6 +8,7 @@
ClangdUnit.cpp
ClangdUnitStore.cpp
CodeComplete.cpp
+ Context.cpp
Compiler.cpp
DraftStore.cpp
FuzzyMatch.cpp
Index: clangd/Context.h
===
--- clangd/Context.h
+++ clangd/Context.h
@@ -0,0 +1,186 @@
+//===--- Context.h - Mechanism for passing implicit data *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Context for storing and retrieving implicit data. Useful for passing implicit
+// parameters on a per-request basis.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+
+#include "llvm/ADT/STLExtras.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+/// A key for a value of type \p Type, stored inside a context. Keys are
+/// non-movable and non-copyable. See documentation of the Context class for
+/// more details and usage examples.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// 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, 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.
+///
+/// C
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 of a reference.
- Renamed ContextData to Data.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,57 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(ContextTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ Context Ctx = Context::empty().derive(IntParam, 10).derive(ExtraIntParam, 20);
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(ContextTests, MoveOps) {
+ Key> Param;
+
+ Context Ctx = Context::empty().derive(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ Context NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ Context::empty().derive(ParentParam, 10).derive(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive(ParentAndChildParam, 30).derive(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/Context.h
===
--- /dev/null
+++ clangd/Context.h
@@ -0,0 +1,186 @@
+//===--- Context.h - Mechanism for passing implicit data *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Context for storing and retrieving implicit data. Useful for passing implicit
+// parameters on a per-request basis.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+
+#include "llvm/ADT/STLExtras.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+/// A key for a value of type \p Type, stored inside a context. Keys are
+/// non-movable and non-copyable. See documentation of the Context class for
+/// more details and usage examples.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// 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, 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 result
+///callback of async operations will receive the context again.
+/// - cloned only when 'forking' an asynchronous computation that we don't wait
+///for.
+///
+/// Copy ope
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 wrote:
> Is this comment still true/relevant?
> I thought the motivating case was Span, but Span now stores a copy of the
> parent pointer (and ContextData isn't accessible by it).
I'd argue we still want to keep this invariant, it gives a natural order of
destruction.
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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();
+
as discussed offline, this could also return a value, which might make some use
cases (testing) slightly cleaner. Up to you
Comment at: clangd/Context.h:109
+ template const Type *get(const Key &Key) const {
+const ContextData *DataPtr = this->DataPtr.get();
+while (DataPtr != nullptr) {
nit: this is a for loop in disguise :-)
Comment at: clangd/Context.h:131
+ template
+ Context derive(const Key &Key, Args &&... As) const & {
+return Context(std::make_shared(ContextData{
I'd find the interface (and in particular the error messages) here easier to
understand if we took a `Type` by value, rather than having `emplace` semantics.
If this function took a `Type`, and `TypedAnyStorage` took a `Type&&`, the cost
would be one extra move, which doesn't seem too bad.
Up to you, though. (If you change it, don't forget the other `derive` overload)
Comment at: clangd/Context.h:168
+
+ struct ContextData {
+// We need to make sure Parent outlives the Value, so the order of members
nit: now this is private it could just be called Data
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
Is this comment still true/relevant?
I thought the motivating case was Span, but Span now stores a copy of the
parent pointer (and ContextData isn't accessible by it).
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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-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. > > I don't see temporary unnamed `Context`s being any different from temporary > > unnamed `ContextBuilder`s. > > > > But we've gone around on this point a bit, and this really seems to be a > > question of taste. @klimek, can we have a third opinion? > > > > The options we're looking at are: > > - `Context` stores a map and a parent pointer. `derive()` returns a > > `ContextBuilder` used to create new contexts containing 0 or more new KV > > pairs. `TypedValueMap` stores the payloads. > > - `Context` stores a single KV pair and a parent pointer. `derive(K, V)` > > is used to create a new context with one new KV pair. A Key-pointer and > > AnyStorage in `Context` store the payloads, the rest of `TypedValueMap` > > goes away. > I'd agree that Context::derive(K, V) would be simpler here, mainly because > there is only one way to pass around contexts while we're building them up; > specifically, there's no temptation to pass around ContextBuilders. Done. `ContextBuilder` and `TypedValueMap` are now removed. 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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,57 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(ContextTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ Context Ctx = Context::empty().derive(IntParam, 10).derive(ExtraIntParam, 20);
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(ContextTests, MoveOps) {
+ Key> Param;
+
+ Context Ctx = Context::empty().derive(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ Context NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ Context::empty().derive(ParentParam, 10).derive(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive(ParentAndChildParam, 30).derive(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/Context.h
===
--- /dev/null
+++ clangd/Context.h
@@ -0,0 +1,183 @@
+//===--- Context.h - Mechanism for passing implicit data *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Context for storing and retrieving implicit data. Useful for passing implicit
+// parameters on a per-request basis.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONTEXT_H_
+
+#include "llvm/ADT/STLExtras.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+/// A key for a value of type \p Type, stored inside a context. Keys are
+/// non-movable and non-copyable. See documentation of the Context class for
+/// more details and usage examples.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// 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, 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 result
+///callback of async operations will receive the context again.
+/// - cloned only when 'forking' an asynchronous computation that we don't wait
+///for.
+///
+/// Copy operations for this class are deleted, use an explicit clone() method
+/// when you need a copy of the context instead.
+///
+/// To derive a child context use derive() function, e.g.
+/// Context C
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 (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.
> I don't see temporary unnamed `Context`s being any different from temporary
> unnamed `ContextBuilder`s.
>
> But we've gone around on this point a bit, and this really seems to be a
> question of taste. @klimek, can we have a third opinion?
>
> The options we're looking at are:
> - `Context` stores a map and a parent pointer. `derive()` returns a
> `ContextBuilder` used to create new contexts containing 0 or more new KV
> pairs. `TypedValueMap` stores the payloads.
> - `Context` stores a single KV pair and a parent pointer. `derive(K, V)` is
> used to create a new context with one new KV pair. A Key-pointer and
> AnyStorage in `Context` store the payloads, the rest of `TypedValueMap` goes
> away.
I'd agree that Context::derive(K, V) would be simpler here, mainly because
there is only one way to pass around contexts while we're building them up;
specifically, there's no temptation to pass around ContextBuilders.
Comment at: clangd/TypedValueMap.h:38
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
Should we say "from Key*" or &? If you say Key I expect something that
compares by value.
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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/Context.h
clangd/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,73 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx = Context::empty()
+ .derive()
+ .add(ParentParam, 10)
+ .add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return A
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 kill me some day. Added the comment, too. Although that should be obvious from the `&&` in the signature. 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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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: > 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. I don't see temporary unnamed `Context`s being any different from temporary unnamed `ContextBuilder`s. But we've gone around on this point a bit, and this really seems to be a question of taste. @klimek, can we have a third opinion? The options we're looking at are: - `Context` stores a map and a parent pointer. `derive()` returns a `ContextBuilder` used to create new contexts containing 0 or more new KV pairs. `TypedValueMap` stores the payloads. - `Context` stores a single KV pair and a parent pointer. `derive(K, V)` is used to create a new context with one new KV pair. A Key-pointer and AnyStorage in `Context` store the payloads, the rest of `TypedValueMap` goes away. Comment at: clangd/Context.h:142 +/// Creates a ContextBuilder with a null parent. +ContextBuilder buildCtx(); + ilya-biryukov wrote: > 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`? I think this isn't a path we want to make smooth or obvious - usually what you want is to accept a context, derive from it, and use the result. Embedders will need to create root contexts - we discussed `LSPServer` offline, and there it seems natural to own a background context and derive from it per handled call. Comment at: clangd/Context.h:92 + ContextBuilder derive() const &; + ContextBuilder derive() const &&; + `&&`, not `const&&` :-) Maybe add a trailing `//takes ownership` 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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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, 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 Type *get(Key &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 Type &getExisting(Key &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 `emptyC
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return false;
+
+
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
clangd/Context.h
clangd/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return false;
+
+Map.erase(It);
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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/Context.cpp
clangd/Context.h
clangd/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.fi
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,104 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template const Type *get(const Key &Key) const {
+return const_cast(this)->get(Key);
+ }
+
+ template Type *get(const Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ r
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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.
- Made derive() const.
- Added docs.
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/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,100 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TYPEDVALUEMAP_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template Type *get(const Key &Key) const {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(const Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ te
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 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... :(
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
+/// used as parents for some other Contexts.
+class Context {
+public:
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.
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
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 agree they're complex! Happy to pull
in a third opinion if you think we can't or shouldn't live without them.
Comment at: clangd/Context.h:11
+// Context for storing and retrieving implicit data. Useful for passing
implicit
+// parameters on a per-request basis.
+//
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, 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.
Comment at: clangd/Context.h:25
+
+class ContextData {
+public:
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.
Comment at: clangd/Context.h:32
+ /// specified for \p Key, return null.
+ template Type *get(Key &Key) const {
+if (auto Val = Data.get(Key))
nit: const Key&, and below
Comment at: clangd/Context.h:32
+ /// specified for \p Key, return null.
+ template Type *get(Key &Key) const {
+if (auto Val = Data.get(Key))
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.
Comment at: clangd/Context.h:42
+ /// Must not be called for keys that are not in the map.
+ template Type &getExisting(Key &Key) const {
+auto Val = get(Key);
I'd prefer the standard name `at` for this function (assert vs throw being
pretty minor I think), but YMMV
Comment at: clangd/Context.h:49
+
+ /// A helper to get a string value as StringRef. Returns empty StringRef if
\p
+ /// StrKey does not exist in a map.
This is unused - remove until we need it?
Comment at: clangd/Context.h:61
+ /// exist in a map.
+ template Type *getPtr(Key &Key) const {
+if (auto Val = get(Key))
This is only used where getExisting() would be better - remove until we need it?
Comment at: clangd/Context.h:68
+private:
+ // We need to make sure Parent is destroyed after Data. The order of members
+ // is important.
Nit: is destroyed after --> outlives?
If you're going to repeat that this is important, then say why :-)
Comment at: clangd/Context.h:82
+/// used as parents for some other Contexts.
+class Context {
+public:
If we're going to define this as a real class (rather than use/inherit
`shared_ptr`), we should really be able to put the API on it directly (rather
than require the user to `->`)
Comment at: clangd/Context.h:96
+
+ ContextBuilder derive();
+
If we discourage copies, you probably want a moving `ContextBuilder derive()
&&` overload
Comment at: clangd/Context.h:96
+
+ ContextBuilder derive();
+
sammccall wrote:
> If we discourage copies, you probably want a moving `ContextBuilder derive()
> &&` overload
nit: const
Comment at: clangd/Context.h:139
+/// that require a Context when no explicit Context is not available.
+Context &emptyCtx();
+
nit: how do you feel about Context::empty()? it ties the name to the type more
clearly, I think.
Comment at: clangd/Context.h:139
+/// that require
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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.org/D40485
Files:
clangd/CMakeLists.txt
clangd/Context.cpp
clangd/Context.h
clangd/TypedValueMap.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx->get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx->get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx->get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx->get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx->get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx->get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,95 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template Type *get(Key &Key) const {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return false;
+
+Map.erase(It);
+return true;
+ }
+
+private:
+ class AnyStorage {
+ public:
+virtual ~AnyS
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 GlobalSession {
+public:
sammccall wrote:
> Maybe `WithGlobalContext` is a good name for this scoped object?
>
> This comment doesn't give a clear idea why someone would want to call this.
> Maybe `All contexts used by clangd inherit from this global context
> (including contexts created internally)`
Remove the global context altogether.
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > bkramer wrote:
> > > > This is a giant code smell. If we want the context route, please pass
> > > > contexts everywhere. I really don't want this kind of technical debt in
> > > > clangd now.
> > > I'm with you on this one, but I think @sammccall was keen on having the
> > > global context. The main reason was to always have access to **some**
> > > loggers and tracers, even when it's hard to pass the Context into the
> > > function.
> > > It's perfectly easy to remove all usages of `globalCtx()`, currently only
> > > 8 usages to get rid of. However, I'd wait for Sam's comment before doing
> > > that.
> > It's important to be able to call functions that require a context if you
> > don't have one - adding a log statement/trace for debugging shouldn't
> > require changing plumbing/interfaces. (It's fine if we want to avoid
> > checking in such code...)
> > Having an "empty" global context allows this.
> >
> > At the same time, we want the ability to set the sink for logs/traces etc
> > globally.
> >
> > A couple of options:
> > - the sink (e.g. Logger) is part of the context, we need to allow
> > embedders to set/augment the global context
> > - the sink is not stored in the context, instead it is some other
> > singleton the embedder can set up
> >
> > I don't have a strong opinion which is better. It's nice to reuse
> > mechanisms, on the other hand loggers vs request IDs are pretty different
> > types of data.
> I'd still get rid of it. The less implicit behavior we have, the better.
> It does not seem hard to plumb the `Context` all the way through clangd
> currently. And it should not be too hard in the future.
>
> I was asking myself multiple times whether we needed the global context in
> the first place while implementing it.
> I think we should aim for avoiding global state altogether. That includes
> singletons, etc.
Removed the global context.
Comment at: clangd/Context.h:116
+/// Creates a new ContextBuilder, using globalCtx() as a parent.
+ContextBuilder buildCtx();
+/// Creates a new ContextBuilder with explicit \p Parent.
ilya-biryukov wrote:
> sammccall wrote:
> > This seems more naturally a method on Context, e.g.
> >
> > Context C = globalCtx().derive(key, value);
> >
> > The relationship between global and C is clear.
> >
> > (You can allow chaining+mapping by having Context::derive and
> > ContextBuilder::derive both return ContextBuilder&&, but as noted below I'm
> > not sure it's worth the complexity over Context::derive ->Context)
> I think the current interface is simple enough and allows for both storage
> implementations. I'd really avoid providing an interface that ties us into a
> single storage implementation.
Added a `derive()` method to the `Context`. It currently returns
`ContextBuilder`, I still think it's nice to have the `ContextBuilder` and a
separation between an immutable `Context` and a mutable `ContextBuilder`. Happy
to chat on whether we want to remove it.
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
ilya-biryukov wrote:
> sammccall wrote:
> > This might be doing a little more than it needs to.
> >
> > Do we need the ability to have multiple values of the same type?
> > If request ID is an int, needing to do `struct RequestID { ID int };`
> > doesn't seem like much of a burden, and simplifies the semantics here.
> >
> > And for cases like Logger where the type carries the semantics, keying by
> > type is clearer.
> Are `Key<>` types confusing? I really like the fact that I don't have to
> specify type information and it is carried in the `Key` to the
> `get`/`emplace` methods, i.e. I don't have to specify template arguments
> there.
> I don't see how `struct RequestId { ID int };` is clearer or shorter than
> `Key RequestID;`. Again, are `Key` a confusing concept in the
> first place?
Unfortunately, `typeid` does not with `-fno-rtti`, so there does not seem to be
a way to significantly simplify the implementation (i.e. we'll ne
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 used types to Context.
- Removed PtrKey, this simplified code a bit.
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/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,71 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ ParentCtx.derive().add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx->get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx->get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx->get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx->get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx->get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx->get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
CodeCompleteTests.cpp
+ ContextTests.cpp
FuzzyMatchTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,95 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template Type *get(Key &Key) const {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(Key &Key, Args &&... As) {
+bool Added =
+Map.try_emplace(&Key,
+llvm::make_unique<
+TypedAnyStorage::type>>(
+std::forward(As)...))
+.second;
+return Added;
+ }
+
+ template bool remove(Key &Key) {
+auto It = Map.find(&Key);
+if (It == Map.end()
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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)
> - GlobalSession swapped its context with Global, and then swapped back in
> its destructor
Will do
Comment at: clangd/Context.h:65
+ Context *Parent;
+ TypedValueMap Data;
+};
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.
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
sammccall wrote:
> ilya-biryukov wrote:
> > bkramer wrote:
> > > This is a giant code smell. If we want the context route, please pass
> > > contexts everywhere. I really don't want this kind of technical debt in
> > > clangd now.
> > I'm with you on this one, but I think @sammccall was keen on having the
> > global context. The main reason was to always have access to **some**
> > loggers and tracers, even when it's hard to pass the Context into the
> > function.
> > It's perfectly easy to remove all usages of `globalCtx()`, currently only 8
> > usages to get rid of. However, I'd wait for Sam's comment before doing that.
> It's important to be able to call functions that require a context if you
> don't have one - adding a log statement/trace for debugging shouldn't require
> changing plumbing/interfaces. (It's fine if we want to avoid checking in such
> code...)
> Having an "empty" global context allows this.
>
> At the same time, we want the ability to set the sink for logs/traces etc
> globally.
>
> A couple of options:
> - the sink (e.g. Logger) is part of the context, we need to allow embedders
> to set/augment the global context
> - the sink is not stored in the context, instead it is some other singleton
> the embedder can set up
>
> I don't have a strong opinion which is better. It's nice to reuse mechanisms,
> on the other hand loggers vs request IDs are pretty different types of data.
I'd still get rid of it. The less implicit behavior we have, the better.
It does not seem hard to plumb the `Context` all the way through clangd
currently. And it should not be too hard in the future.
I was asking myself multiple times whether we needed the global context in the
first place while implementing it.
I think we should aim for avoiding global state altogether. That includes
singletons, etc.
Comment at: clangd/Context.h:116
+/// Creates a new ContextBuilder, using globalCtx() as a parent.
+ContextBuilder buildCtx();
+/// Creates a new ContextBuilder with explicit \p Parent.
sammccall wrote:
> This seems more naturally a method on Context, e.g.
>
> Context C = globalCtx().derive(key, value);
>
> The relationship between global and C is clear.
>
> (You can allow chaining+mapping by having Context::derive and
> ContextBuilder::derive both return ContextBuilder&&, but as noted below I'm
> not sure it's worth the complexity over Context::derive ->Context)
I think the current interface is simple enough and allows for both storage
implementations. I'd really avoid providing an interface that ties us into a
single storage implementation.
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
sammccall wrote:
> This might be doing a little more than it needs to.
>
> Do we need the ability to have multiple values of the same type?
> If request ID is an int, needing to do `struct RequestID { ID int };` doesn't
> seem like much of a burden, and simplifies the semantics here.
>
> And for cases like Logger where the type carries the semantics, keying by
> type is clearer.
Are `Key<>` types confusing? I really like the fact that I don't have to
specify type information and it is carried in the `Key` to the
`get`/`emplace` methods, i.e. I don't have to spe
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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.
Comment at: clangd/Context.cpp:16
+
+static Context *GlobalCtx = nullptr;
+static Context EmptyContext(nullptr, {});
Seems like it would simplify things if:
- GlobalCtx was always set (static local trick)
- GlobalSession swapped its context with Global, and then swapped back in its
destructor
Comment at: clangd/Context.h:64
+private:
+ Context *Parent;
+ TypedValueMap Data;
nit: const
Comment at: clangd/Context.h:65
+ Context *Parent;
+ TypedValueMap Data;
+};
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.
Comment at: clangd/Context.h:71
+/// before any clangd functions are called.
+class GlobalSession {
+public:
Maybe `WithGlobalContext` is a good name for this scoped object?
This comment doesn't give a clear idea why someone would want to call this.
Maybe `All contexts used by clangd inherit from this global context (including
contexts created internally)`
Comment at: clangd/Context.h:79
+/// Otherwise returns an empty Context.
+Context &globalCtx();
+
ilya-biryukov wrote:
> bkramer wrote:
> > This is a giant code smell. If we want the context route, please pass
> > contexts everywhere. I really don't want this kind of technical debt in
> > clangd now.
> I'm with you on this one, but I think @sammccall was keen on having the
> global context. The main reason was to always have access to **some** loggers
> and tracers, even when it's hard to pass the Context into the function.
> It's perfectly easy to remove all usages of `globalCtx()`, currently only 8
> usages to get rid of. However, I'd wait for Sam's comment before doing that.
It's important to be able to call functions that require a context if you don't
have one - adding a log statement/trace for debugging shouldn't require
changing plumbing/interfaces. (It's fine if we want to avoid checking in such
code...)
Having an "empty" global context allows this.
At the same time, we want the ability to set the sink for logs/traces etc
globally.
A couple of options:
- the sink (e.g. Logger) is part of the context, we need to allow embedders to
set/augment the global context
- the sink is not stored in the context, instead it is some other singleton
the embedder can set up
I don't have a strong opinion which is better. It's nice to reuse mechanisms,
on the other hand loggers vs request IDs are pretty different types of data.
Comment at: clangd/Context.h:116
+/// Creates a new ContextBuilder, using globalCtx() as a parent.
+ContextBuilder buildCtx();
+/// Creates a new ContextBuilder with explicit \p Parent.
This seems more naturally a method on Context, e.g.
Context C = globalCtx().derive(key, value);
The relationship between global and C is clear.
(You can allow chaining+mapping by having Context::derive and
ContextBuilder::derive both return ContextBuilder&&, but as noted below I'm not
sure it's worth the complexity over Context::derive ->Context)
Comment at: clangd/TypedValueMap.h:1
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
This might be doing a little more than it needs to.
Do we need the ability to have multiple values of the same type?
If request ID is an int, needing to do `struct RequestID { ID int };` doesn't
seem like much of a burden, and simplifies the semantics here.
And for cases like Logger where the type carries the semantics, keying by type
is clearer.
Comment at: clangd/TypedValueMap.h:76
+
+ template bool emplace(PtrKey &PtrKey, Arg *A) {
+return emplace(PtrKey.UnderlyingKey, A);
Shouldn't this just be Type *A?
I think the extra convenience of PtrKey probably isn't worth the complexity,
though.
https://reviews.llvm.org/D40485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 want this kind of technical debt in > clangd now. I'm with you on this one, but I think @sammccall was keen on having the global context. The main reason was to always have access to **some** loggers and tracers, even when it's hard to pass the Context into the function. It's perfectly easy to remove all usages of `globalCtx()`, currently only 8 usages to get rid of. However, I'd wait for Sam's comment before doing that. https://reviews.llvm.org/D40485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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 context route, please pass contexts everywhere. I really don't want this kind of technical debt in clangd now. https://reviews.llvm.org/D40485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40485: [clangd] Introduced a Context that stores implicit data
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
unittests/clangd/CMakeLists.txt
unittests/clangd/ContextTests.cpp
Index: unittests/clangd/ContextTests.cpp
===
--- /dev/null
+++ unittests/clangd/ContextTests.cpp
@@ -0,0 +1,88 @@
+//===-- ContextTests.cpp - Context tests *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Context.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+TEST(TypedValueMapTests, Simple) {
+ Key IntParam;
+ Key ExtraIntParam;
+
+ clangd::TypedValueMap Ctx;
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 10));
+ ASSERT_TRUE(Ctx.emplace(ExtraIntParam, 20));
+
+ EXPECT_EQ(*Ctx.get(IntParam), 10);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_FALSE(Ctx.emplace(IntParam, 30));
+
+ ASSERT_TRUE(Ctx.remove(IntParam));
+ EXPECT_EQ(Ctx.get(IntParam), nullptr);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+
+ ASSERT_TRUE(Ctx.emplace(IntParam, 30));
+ EXPECT_EQ(*Ctx.get(IntParam), 30);
+ EXPECT_EQ(*Ctx.get(ExtraIntParam), 20);
+}
+
+TEST(TypedValueMapTests, MoveOps) {
+ Key> Param;
+
+ clangd::TypedValueMap Ctx;
+ Ctx.emplace(Param, llvm::make_unique(10));
+ EXPECT_EQ(**Ctx.get(Param), 10);
+
+ clangd::TypedValueMap NewCtx = std::move(Ctx);
+ EXPECT_EQ(**NewCtx.get(Param), 10);
+}
+
+TEST(TypedValueMapTests, PtrKey) {
+ int Value = 10;
+ PtrKey Param;
+
+ clangd::TypedValueMap Ctx;
+ EXPECT_EQ(Ctx.get(Param), nullptr);
+
+ Ctx.emplace(Param, &Value);
+ EXPECT_EQ(*Ctx.get(Param), 10);
+
+ Ctx.remove(Param);
+ EXPECT_EQ(Ctx.get(Param), nullptr);
+
+ Ctx.emplace(Param, nullptr);
+ EXPECT_EQ(Ctx.get(Param), nullptr);
+}
+
+TEST(ContextTests, Builders) {
+ Key ParentParam;
+ Key ParentAndChildParam;
+ Key ChildParam;
+
+ Context ParentCtx =
+ buildCtx().add(ParentParam, 10).add(ParentAndChildParam, 20);
+ Context ChildCtx =
+ buildCtx(&ParentCtx).add(ParentAndChildParam, 30).add(ChildParam, 40);
+
+ EXPECT_EQ(*ParentCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ParentCtx.get(ParentAndChildParam), 20);
+ EXPECT_EQ(ParentCtx.get(ChildParam), nullptr);
+
+ EXPECT_EQ(*ChildCtx.get(ParentParam), 10);
+ EXPECT_EQ(*ChildCtx.get(ParentAndChildParam), 30);
+ EXPECT_EQ(*ChildCtx.get(ChildParam), 40);
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
+ ContextTests.cpp
JSONExprTests.cpp
TraceTests.cpp
)
Index: clangd/TypedValueMap.h
===
--- /dev/null
+++ clangd/TypedValueMap.h
@@ -0,0 +1,123 @@
+//===--- TypedValueMap.h - Type-safe heterogenous key-value map -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Type-safe heterogenous map.
+//
+//===--===//
+
+#include "llvm/ADT/DenseMap.h"
+#include
+
+namespace clang {
+namespace clangd {
+
+/// Used as identity for map values. Non-movable and non-copyable. Address of
+/// this object is used internally to as keys in a map.
+template class Key {
+public:
+ static_assert(!std::is_reference::value,
+"Reference arguments to Key<> are not allowed");
+
+ Key() = default;
+
+ Key(Key const &) = delete;
+ Key &operator=(Key const &) = delete;
+ Key(Key &&) = delete;
+ Key &operator=(Key &&) = delete;
+};
+
+/// Similar to a Key with a slightly easier to use semantics.
+/// While get(Key) returns T**, get(PtrKey) returns T*.
+/// Therefore PtrKey<> does not distinguish values missing from the map and
+/// values equal to null.
+template class PtrKey {
+public:
+ Key UnderlyingKey;
+};
+
+/// A type-safe map from Key to T.
+class TypedValueMap {
+public:
+ TypedValueMap() = default;
+ TypedValueMap(const TypedValueMap &) = delete;
+ TypedValueMap(TypedValueMap &&) = default;
+
+ template Type *get(Key &Key) const {
+auto It = Map.find(&Key);
+if (It == Map.end())
+ return nullptr;
+return static_cast(It->second->getValuePtr());
+ }
+
+ template
+ bool emplace(Key &Key,
