[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Relation.h:1
+//===--- Ref.h ---------------------------------------------------*- C++-*-===//
+//
----------------
gribozavr wrote:
> "--- Relation.h --------------------"
Not done?


================
Comment at: clang-tools-extra/clangd/index/Relation.h:41
+public:
+  // Key.Symbol is Key.Kind of every symbol in Value.
+  // For example, if Key.Kind == SymbolRole::RelationChildOf,
----------------
gribozavr wrote:
> Three slashes for doc comments, please.
Not done?


================
Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+    RelationKey Key;
----------------
nridge wrote:
> gribozavr wrote:
> > Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)
> This comment made me realize that I haven't addressed your previous comment properly: I haven't changed `RelationSlab::value_type` from `std::pair<RelationKey, llvm::ArrayRef<SymbolID>>` to `Relation`.
> 
> I tried to make that change this time, and ran into a problem:
> 
> In the rest of the subtypes patch (D58880), one of the things I do is extend the `MemIndex` constructor so that, in addition to taking a symbol range and a ref range, it takes a relation range.
> 
> That constructor assumes that the elements of that range have members of some name - either `first` and `second` (as currently in D58880), or `Key` and `Value`.
> 
> However, that constructor has two call sites. In `MemIndex::build()`, we pass in the slabs themselves as the ranges. So, if we make this change, the field names for that call site will be `Key` and `Value`. However, for the second call site in `FileSymbols::buildIndex()`, the ranges that are passed in are `DenseMap`s, and therefore their elements' field names are necessarily `first` and `second`. The same constructor cannot therefore accept both ranges.
> 
> How do you propose we address this?
> 
>  * Scrap `struct Relation`, and keep `value_type` as `std::pair<RelationKey, llvm::ArrayRef<SymbolID>>`?
>  * Keep `struct Relation`, but make its fields named `first` and `second`?
>  * Split the constructor of `MemIndex` into two constructors, to accomodate both sets of field names?
>  * Something else?
I guess we should scrap it then.  Kadir, WDYT?


================
Comment at: clang-tools-extra/clangd/index/Relation.h:68
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.
----------------
gribozavr wrote:
> No need to repeat the type name being documented.  "A mutable container that can ..."
Not done?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407