nridge marked 2 inline comments as done. nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); + CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, + Direction)); ---------------- sammccall wrote: > nridge wrote: > > sammccall wrote: > > > relying on the item range to resolve the actual symbol isn't reliable: > > > - the source code may have changed > > > - the range may not be within the TU, and might be e.g. in an indexed > > > header we don't have a compile command for. > > Good point. I appreciate a bit better now why you suggested trying to avoid > > `resolve` for now :) > > > > What do you think of the following implementation approach: > > > > * Use the `data` field of `TypeHierarchyItem` (which the client will send > > back to the server for `resolve` queries) to send the `SymbolID` of the > > item to the client. > > * When the client sends back the `SymbolID` in the `resolve` request, look > > up the symbol in the index, and use the source range from the symbol. > > > > (Later, when we start storing base <--> derived relationships in the index > > for subtype support, we could just answer the entire `resolve` query using > > the index.) > Thanks for exploring all the options here! > > Generally we've tried to avoid relying on the index unless it's needed, using > the AST where possible. There are failure modes here: > - the base type is in the current file, which is actively edited. The ranges > in the index may be off due to staleness. > - the base type is not indexed, because it is e.g. a member class inside a > class template > - there's no index (`-index=0`, though I'm not sure why we still support > this) or the index is stale and the type is missing (we're working on making > index updates more async) > - the base type is not encodable. > > There are just more moving pieces here, I think. Is there a clear reason to > support resolve for parents? > Is there a clear reason to support resolve for parents? Just what I said earlier about a hypothetical client that relies on it. However, given the complications involved in implementing it, I'm happy with only being concerned with actual clients, not hypothetical ones. The only client implementation I currently know of is Theia, and I checked that it works fine without `resolve`, so I'm happy with deferring work on `resolve` for now. If another client comes along that relies on `resolve`, we can revisit this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56370/new/ https://reviews.llvm.org/D56370 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
