kadircet added a comment.
mostly LG, thanks!
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1057
+ auto &CD = S.Definition ? S.Definition : S.CanonicalDeclaration;
+ Start.line = CD.Start.line();
+ Start.character = CD.Start.column();
----------------
nit: could we directly use `THI.range.start.line` (same for the following 3
lines)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1061
+ End.character = CD.End.column();
+ // TODO: How to get entire range like in declToTypeHierarchyItem()?
+ THI.range = {Start, End};
----------------
a few different approaches that comes to my mind:
- store the full range in index.
- check AST cache to see if we have AST for `CD.FileURI`, and use that decl.
- build AST for `CD.FileURI`
- heuristically parse `CD.FileURI`
Could you create a bug report in https://github.com/clangd/clangd/issues ?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1064
+ THI.selectionRange = {Start, End};
+ // TODO: Reuse code between here and getWorkspaceSymbols().
+ auto Uri = URI::parse(CD.FileURI);
----------------
yeah it would be great to have a `Location symbolToLocation(const Symbol& S);`
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1071
+ }
+ // TODO: Pass in ClangdServer::WorkspaceRoot as a HintPath.
+ StringRef HintPath;
----------------
you do not need WorkspaceRoot, you can pass the translationunit
`getTypeHierarchy` was triggered on. Which is available in `File` parameter.
Also it always exists whereas `WorkspaceRoot` might be missing depending on the
editor.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1212
+ if (Index) {
+ if (Optional<SymbolID> ID = getSymbolID(CXXRD)) {
+ fillSubTypes(*ID, *Result->children, Index, ResolveLevels);
----------------
nit: braces(just the inner one, keep the outer one)
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:459
+ Request.AnyScope = true;
+ int ResultCount = 0;
+ Index->fuzzyFind(Request, [&](const Symbol &S) {
----------------
`bool GotResult = false;`
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:462
+ if (TemplateArgs == S.TemplateSpecializationArgs) {
+ Result = S.ID;
+ ++ResultCount;
----------------
```
EXPECT_FALSE(GotResult);
GotResult = true;
```
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:466
+ });
+ EXPECT_EQ(1, ResultCount);
+ return Result;
----------------
`EXPECT_TRUE(GotResult);`
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:470
+
+std::vector<SymbolID> collectSubtypes(SymbolID Type, SymbolIndex *Index) {
+ std::vector<SymbolID> Result;
----------------
Maybe call it `Subject` instead of `Type` ?
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:478
+
+TEST(Subtypes, SimpleInheritance) {
+ Annotations Source(R"cpp(
----------------
could you add another child that derives `Parent` to check multiple children
case?
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:479
+TEST(Subtypes, SimpleInheritance) {
+ Annotations Source(R"cpp(
+struct Parent {
----------------
could you get rid of member fields and put definitions into single lines to
make test smaller?
================
Comment at: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp:505
+TEST(Subtypes, MultipleInheritance) {
+ Annotations Source(R"cpp(
+struct Parent1 {
----------------
same suggestion for member fields and single line definitions
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58880/new/
https://reviews.llvm.org/D58880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
