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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
