sammccall added a comment. In D56370#1391924 <https://reviews.llvm.org/D56370#1391924>, @nridge wrote:
> As far as reworking the tests to use these functions, I've thought about that > a bit: > > - These functions return AST nodes. It's not clear to me how I would come up > with "expected" AST nodes to test the return values against. See FindDecl > - Even if we find a way to get "expected" AST nodes, we would be losing test > coverage of functions like `declToTypeHierarchyItem()` (though I suppose we > could write separate tests for that). > - We could instead test against the //properties// of the AST nodes, such as > their names and source ranges, but then the test code to query those > properties would basically be duplicating code in > `declToTypeHierarchyItem()`. It would seem to make more sense to just test > the resulting `TypeHierarchyItem` instead (as the tests are doing now). ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543 + return CB(InpAST.takeError()); + CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve, + Direction)); ---------------- 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. 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
