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

[Sam McCall via Phabricator via cfe-commits]

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 <class Type> const Type *get(const Key<Type> &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 <class Type, class... Args>
+  Context derive(const Key<Type> &Key, Args &&... As) const & {
+    return Context(std::make_shared<ContextData>(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