[PATCH] D58880: [clangd] Type hierarchy subtypes

[Kadir Cetinkaya via Phabricator via cfe-commits]

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