[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE331189: [clangd] Using index for GoToDefinition. (authored 
by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45717?vs=144561=144566#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717

Files:
  clangd/ClangdServer.cpp
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -13,11 +13,14 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
+#include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
-#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -37,17 +40,33 @@
 };
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-  {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "") {
+  auto HeaderPath = testPath("foo.h");
+  auto MainPath = testPath("foo.cpp");
+  llvm::IntrusiveRefCntPtr VFS(
+  new vfs::InMemoryFileSystem());
+  VFS->addFile(MainPath, 0, llvm::MemoryBuffer::getMemBuffer(MainCode));
+  VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+std::vector args = {"-include", HeaderPath.c_str()};
+Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
+  }
+  auto CI = createInvocationFromCommandLine(Cmd);
+
+  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
-  std::make_shared(),
-  vfs::getRealFileSystem());
+  std::make_shared(), VFS);
   assert(AST.hasValue());
   return std::move(*AST);
 }
 
+std::unique_ptr buildIndex(StringRef MainCode,
+StringRef HeaderCode) {
+  auto AST = build(MainCode, HeaderCode);
+  return MemIndex::build(indexAST());
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -106,6 +125,65 @@
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
+TEST(GoToDefinition, WithIndex) {
+  Annotations SymbolHeader(R"cpp(
+class $forward[[Forward]];
+class $foo[[Foo]] {};
+
+void $f1[[f1]]();
+
+inline void $f2[[f2]]() {}
+  )cpp");
+  Annotations SymbolCpp(R"cpp(
+  class $forward[[forward]] {};
+  void  $f1[[f1]]() {}
+)cpp");
+
+  auto Index = buildIndex(SymbolCpp.code(), SymbolHeader.code());
+  auto runFindDefinitionsWithIndex = [](const Annotations ) {
+auto AST = build(/*MainCode=*/Main.code(),
+ /*HeaderCode=*/"");
+return clangd::findDefinitions(AST, Main.point(), Index.get());
+  };
+
+  Annotations Test(R"cpp(// only declaration in AST.
+void [[f1]]();
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// definition in AST.
+void [[f1]]() {}
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+
+  Test = Annotations(R"cpp(// forward declaration in AST.
+class [[Foo]];
+F^oo* create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// defintion in AST.
+class [[Forward]] {};
+F^orward create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({
+  RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+  }));
+}
+
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -15,13 +15,15 @@
 
 #include "ClangdUnit.h"
 #include "Protocol.h"

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the review!




Comment at: clangd/XRefs.cpp:277
+// it.
+auto ToLSPLocation = [](
+const SymbolLocation ) -> llvm::Optional 
{

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > (The double-nested lambda is somewhat hard to read, can this just be a 
> > > top-level function? That's what's needed to share it, right?)
> > Moved it to `XRef.h`, and also replace the one in `findsymbols`.
> This is pretty weird in terms of layering I think :-(
> The function is pretty trivial, but depends on a bunch of random stuff.
> Can we move it back (and live with the duplication) until we come up with a 
> good home?
Reverted it, made it local in this file and added a FIXME.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 144561.
hokein marked 7 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717

Files:
  clangd/ClangdServer.cpp
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -13,11 +13,14 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
+#include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
-#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -37,17 +40,33 @@
 };
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-  {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "") {
+  auto HeaderPath = testPath("foo.h");
+  auto MainPath = testPath("foo.cpp");
+  llvm::IntrusiveRefCntPtr VFS(
+  new vfs::InMemoryFileSystem());
+  VFS->addFile(MainPath, 0, llvm::MemoryBuffer::getMemBuffer(MainCode));
+  VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+std::vector args = {"-include", HeaderPath.c_str()};
+Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
+  }
+  auto CI = createInvocationFromCommandLine(Cmd);
+
+  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
-  std::make_shared(),
-  vfs::getRealFileSystem());
+  std::make_shared(), VFS);
   assert(AST.hasValue());
   return std::move(*AST);
 }
 
+std::unique_ptr buildIndex(StringRef MainCode,
+StringRef HeaderCode) {
+  auto AST = build(MainCode, HeaderCode);
+  return MemIndex::build(indexAST());
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -106,6 +125,65 @@
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
+TEST(GoToDefinition, WithIndex) {
+  Annotations SymbolHeader(R"cpp(
+class $forward[[Forward]];
+class $foo[[Foo]] {};
+
+void $f1[[f1]]();
+
+inline void $f2[[f2]]() {}
+  )cpp");
+  Annotations SymbolCpp(R"cpp(
+  class $forward[[forward]] {};
+  void  $f1[[f1]]() {}
+)cpp");
+
+  auto Index = buildIndex(SymbolCpp.code(), SymbolHeader.code());
+  auto runFindDefinitionsWithIndex = [](const Annotations ) {
+auto AST = build(/*MainCode=*/Main.code(),
+ /*HeaderCode=*/"");
+return clangd::findDefinitions(AST, Main.point(), Index.get());
+  };
+
+  Annotations Test(R"cpp(// only declaration in AST.
+void [[f1]]();
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// definition in AST.
+void [[f1]]() {}
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+
+  Test = Annotations(R"cpp(// forward declaration in AST.
+class [[Foo]];
+F^oo* create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// defintion in AST.
+class [[Forward]] {};
+F^orward create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({
+  RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+  }));
+}
+
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -71,6 +71,10 @@
   MemIndex Index;
 };
 
+/// Retrieves namespace and class level symbols in \p AST.
+/// Exposed to assist in unit tests.
+SymbolSlab 

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

A few more nits (and some ideas for further restructuring the merge logic).
Otherwise LG, let's land this!




Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {

hokein wrote:
> sammccall wrote:
> > So now you're early-out if there are macros.
> > This means if you have
> > 
> > ```
> > void log(string s) { cout << s; }
> > #define LOG log
> > LOG("hello");
> > ```
> > 
> > You'll offer only line 2 as an option, and not line 1.
> > I'm not sure that's bad, but it's non-obvious - I think it's the thing that 
> > the comment should call out.
> > e.g. "If the cursor is on a macro, go to the macro's definition without 
> > trying to resolve the code it expands to"
> > (The current comment just echoes the code)
> This is a non-functional change.
> 
> For the above example, only line 2 will be offered. This is expected behavior 
> IMO -- when we go to definition on a macro, users would expect the macro 
> definition.
Ah, so these cases can't both happen (we even have an assert elsewhere).

Still, we can treat them as independent, and it seems simpler: you can remove 
the if statement, the comment for the if statement, and the early return.



Comment at: clangd/XRefs.cpp:237
+  //(e.g. function declaration in header), and a location of definition if
+  //they are available, definition comes first.
+  struct CandidateLocation {

hokein wrote:
> sammccall wrote:
> > first why?
> because this is `go to definition`, so it is sensible to return the 
> `definition` as the first result, right?
Again, I don't think "definition" in LSP is being used in its technical c++ 
sense.
No issue with the behavior here, but I think the comment should be "we think 
that's what the user wants most often" or something - it's good to know the 
reasoning in case we want to change the behavior in the future.



Comment at: clangd/XRefs.cpp:277
+// it.
+auto ToLSPLocation = [](
+const SymbolLocation ) -> llvm::Optional 
{

hokein wrote:
> sammccall wrote:
> > (The double-nested lambda is somewhat hard to read, can this just be a 
> > top-level function? That's what's needed to share it, right?)
> Moved it to `XRef.h`, and also replace the one in `findsymbols`.
This is pretty weird in terms of layering I think :-(
The function is pretty trivial, but depends on a bunch of random stuff.
Can we move it back (and live with the duplication) until we come up with a 
good home?



Comment at: clangd/XRefs.cpp:244
+  //final results. When there are duplicate results, we prefer AST over
+  //index because AST is more up-to-update.
+  //

up-to-update -> up-to-date



Comment at: clangd/XRefs.cpp:257
+  //   4. Return all populated locations for all symbols, definition first.
+  struct CandidateLocation {
+llvm::Optional ID;

OK, as discussed offline the bit that remains confusing is why this isn't a map.
The reason is that some symbols don't have USRs and therefore no symbol IDs.

In practice we don't really expect  multiple symbol results at all, so what 
about one of:
 - `map`
 - `map` and use `SymbolID("")` as a sentinel

I slightly prefer the latter because it minimizes the invasiveness of handling 
this rare/unimportant case.



Comment at: clangd/index/FileIndex.h:26
 
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);

nit: prefer to remove `\brief` in favor of formatting the comment so the 
summary gets extracted.



Comment at: clangd/index/FileIndex.h:26
 
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);

sammccall wrote:
> nit: prefer to remove `\brief` in favor of formatting the comment so the 
> summary gets extracted.
This isn't the main interface of the file.
Move to the bottom, and note that this is exposed to assist in unit tests?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the useful comments! I refined the patch, and it becomes a bit 
larger (including some moving stuff).




Comment at: clangd/XRefs.cpp:137
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST , SourceLocation Pos) {
+  auto DeclMacrosFinder = std::make_shared(

sammccall wrote:
> this is a nice abstraction - consider hiding the DeclarationAndMacrosFinder 
> inside it!
Inlining DeclarationAndMacrosFinder implementation inside this function would 
hurt the code readability, I'd leave it as it is. Now all client sides are 
using this function instead of  `DeclarationAndMacrosFinder`.



Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {

sammccall wrote:
> So now you're early-out if there are macros.
> This means if you have
> 
> ```
> void log(string s) { cout << s; }
> #define LOG log
> LOG("hello");
> ```
> 
> You'll offer only line 2 as an option, and not line 1.
> I'm not sure that's bad, but it's non-obvious - I think it's the thing that 
> the comment should call out.
> e.g. "If the cursor is on a macro, go to the macro's definition without 
> trying to resolve the code it expands to"
> (The current comment just echoes the code)
This is a non-functional change.

For the above example, only line 2 will be offered. This is expected behavior 
IMO -- when we go to definition on a macro, users would expect the macro 
definition.



Comment at: clangd/XRefs.cpp:237
+  //(e.g. function declaration in header), and a location of definition if
+  //they are available, definition comes first.
+  struct CandidateLocation {

sammccall wrote:
> first why?
because this is `go to definition`, so it is sensible to return the 
`definition` as the first result, right?



Comment at: clangd/XRefs.cpp:256
+CandidateLocation  = ResultCandidates[ID];
+bool IsDef = GetDefinition(D) == D;
+// Populate one of the slots with location for the AST.

sammccall wrote:
> why do you not use GetDefinition(D) as the definition, in the case where it's 
> not equal to D?
Done. Added a comment.



Comment at: clangd/XRefs.cpp:277
+// it.
+auto ToLSPLocation = [](
+const SymbolLocation ) -> llvm::Optional 
{

sammccall wrote:
> (The double-nested lambda is somewhat hard to read, can this just be a 
> top-level function? That's what's needed to share it, right?)
Moved it to `XRef.h`, and also replace the one in `findsymbols`.



Comment at: clangd/XRefs.cpp:516
 
   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
   auto DeclMacrosFinder = std::make_shared(

sammccall wrote:
> can you also use `getSymbolAtPosition` here?
Yeah, I just missed this.



Comment at: unittests/clangd/XRefsTests.cpp:43
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+StringRef BaseName = "Main",
+llvm::IntrusiveRefCntPtr

sammccall wrote:
> why allow BaseName to be chosen?
The same reason (allowed us to use the same FS between the index and AST).



Comment at: unittests/clangd/XRefsTests.cpp:44
+StringRef BaseName = "Main",
+llvm::IntrusiveRefCntPtr
+InMemoryFileSystem = nullptr) {

sammccall wrote:
> why do you need to pass in the FS? The FS shouldn't need to be the same 
> between the index and AST.
I intended to use one FS for index and AST, because we'd like to test the AST 
case which includes the index header file, but it turns out this is not needed 
and add complexity of the testcode, removed it.



Comment at: unittests/clangd/XRefsTests.cpp:52
+  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+InMemoryFileSystem->addFile(HeaderPath, 0,

sammccall wrote:
> why do this conditionally?
It is unnecessary if no header code is provided. And `-include` option would 
affect the test somehow, if we do it unconditionally, it breaks one of the 
existing test cases (no location was found). I haven't digged into the reason...

```cpp
int [[i]];
#define ADDRESSOF(X) 
int *j = ADDRESSOF(^i);
```



Comment at: unittests/clangd/XRefsTests.cpp:72
 
+std::unique_ptr buildIndexFromAST(ParsedAST *AST) {
+  SymbolCollector::Options CollectorOpts;

sammccall wrote:
> can we reuse FileIndex instead of reimplementing it?
Shared the implementation from FileIndex.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 143728.
hokein marked 14 inline comments as done.
hokein added a comment.

Address review comments:

- clarify the flow of go to definition.
- simplify the test code.
- some function move-out stuff.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717

Files:
  clangd/ClangdServer.cpp
  clangd/FindSymbols.cpp
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -13,11 +13,14 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
+#include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
-#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -37,17 +40,33 @@
 };
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-  {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "") {
+  auto HeaderPath = testPath("foo.h");
+  auto MainPath = testPath("foo.cpp");
+  llvm::IntrusiveRefCntPtr VFS(
+  new vfs::InMemoryFileSystem());
+  VFS->addFile(MainPath, 0, llvm::MemoryBuffer::getMemBuffer(MainCode));
+  VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+std::vector args = {"-include", HeaderPath.c_str()};
+Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
+  }
+  auto CI = createInvocationFromCommandLine(Cmd);
+
+  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
-  std::make_shared(),
-  vfs::getRealFileSystem());
+  std::make_shared(), VFS);
   assert(AST.hasValue());
   return std::move(*AST);
 }
 
+std::unique_ptr buildIndex(StringRef MainCode,
+StringRef HeaderCode) {
+  auto AST = build(MainCode, HeaderCode);
+  return MemIndex::build(indexAST());
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -106,6 +125,66 @@
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
+TEST(GoToDefinition, WithIndex) {
+  Annotations SymbolHeader(R"cpp(
+class $forward[[Forward]];
+class $foo[[Foo]] {};
+
+void $f1[[f1]]();
+
+inline void $f2[[f2]]() {}
+  )cpp");
+  Annotations SymbolCpp(R"cpp(
+  class $forward[[forward]] {};
+  void  $f1[[f1]]() {}
+)cpp");
+
+  auto Index = buildIndex(SymbolCpp.code(), SymbolHeader.code());
+  auto runFindDefinitionsWithIndex =
+  [](const Annotations ) {
+auto AST = build(/*MainCode=*/Main.code(),
+ /*HeaderCode=*/"");
+return clangd::findDefinitions(AST, Main.point(), Index.get());
+  };
+
+  Annotations Test(R"cpp(// only declaration in AST.
+void [[f1]]();
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// definition in AST.
+void [[f1]]() {}
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+
+  Test = Annotations(R"cpp(// forward declaration in AST.
+class [[Foo]];
+F^oo* create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// defintion in AST.
+class [[Forward]] {};
+F^orward create();
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({
+  RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+  }));
+}
+
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -23,6 +23,9 @@
 

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for the changes! I still find the logic quite confusing. Happy to chat 
offline if useful.




Comment at: clangd/XRefs.cpp:137
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST , SourceLocation Pos) {
+  auto DeclMacrosFinder = std::make_shared(

this is a nice abstraction - consider hiding the DeclarationAndMacrosFinder 
inside it!



Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {

So now you're early-out if there are macros.
This means if you have

```
void log(string s) { cout << s; }
#define LOG log
LOG("hello");
```

You'll offer only line 2 as an option, and not line 1.
I'm not sure that's bad, but it's non-obvious - I think it's the thing that the 
comment should call out.
e.g. "If the cursor is on a macro, go to the macro's definition without trying 
to resolve the code it expands to"
(The current comment just echoes the code)



Comment at: clangd/XRefs.cpp:226
 
-  std::vector Decls = DeclMacrosFinder->takeDecls();
-  std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+  // Handle goto definition for typical declarations.
+  //

I'm not sure this line adds anything unless you are more specific about what 
"typical" means, maybe just drop it.

(nit: go to, not goto)



Comment at: clangd/XRefs.cpp:233
+  //  - We use the location from AST and index (if available) to provide the
+  //final results. Prefer AST over index.
+  //

because it's more up-to-date?



Comment at: clangd/XRefs.cpp:235
+  //
+  //  - For each symbol, we will return a location of the cannonical 
declaration
+  //(e.g. function declaration in header), and a location of definition if

nit: canonical



Comment at: clangd/XRefs.cpp:237
+  //(e.g. function declaration in header), and a location of definition if
+  //they are available, definition comes first.
+  struct CandidateLocation {

first why?



Comment at: clangd/XRefs.cpp:246
+
+  for (auto D : Symbols.Decls) {
+llvm::SmallString<128> USR;

nit: the body of this loop handles building the query and getting the AST 
locations, but they're interleaved in a confusing way - please separate them, 
or even make these two separate loops.

In the latter case you could put QueryRequest inside the if(Index) block, which 
would be easier to follow.



Comment at: clangd/XRefs.cpp:248
+llvm::SmallString<128> USR;
+if (index::generateUSRForDecl(D, USR))
+  continue;

why are we skipping the AST location if we can't generate a USR?



Comment at: clangd/XRefs.cpp:256
+CandidateLocation  = ResultCandidates[ID];
+bool IsDef = GetDefinition(D) == D;
+// Populate one of the slots with location for the AST.

why do you not use GetDefinition(D) as the definition, in the case where it's 
not equal to D?



Comment at: clangd/XRefs.cpp:265
+  if (Index) {
+log("query index...");
+std::string HintPath;

remove this log message or make it more useful :-)



Comment at: clangd/XRefs.cpp:277
+// it.
+auto ToLSPLocation = [](
+const SymbolLocation ) -> llvm::Optional 
{

(The double-nested lambda is somewhat hard to read, can this just be a 
top-level function? That's what's needed to share it, right?)



Comment at: clangd/XRefs.cpp:516
 
   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
   auto DeclMacrosFinder = std::make_shared(

can you also use `getSymbolAtPosition` here?



Comment at: unittests/clangd/XRefsTests.cpp:42
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-  {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+StringRef BaseName = "Main",

this is too complicated, please find a way to make it simpler.
Take a look in TestFS.h for the filesystem stuff.



Comment at: unittests/clangd/XRefsTests.cpp:43
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+StringRef BaseName = "Main",
+llvm::IntrusiveRefCntPtr

why allow BaseName to be chosen?



Comment at: unittests/clangd/XRefsTests.cpp:44
+StringRef BaseName = "Main",
+llvm::IntrusiveRefCntPtr
+

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I have updated the patch according to offline discussion -- for each symbol, we 
will return both decl and def locations (if available, def first) as they seems 
to be most sensible to users. We always prefer location from AST over Index 
when conflicts.




Comment at: clangd/XRefs.cpp:261
+
+  for (auto It : ResultCandidates)
+Result.push_back(It.second);

sammccall wrote:
> Here we're returning exactly one candidate per symbol we identified as a 
> candidate. There are other choices - why this one?
Yeah, this was one **definition** location per  symbol, made a change according 
to our discussion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 143508.
hokein marked 7 inline comments as done.
hokein added a comment.

Refine the patch and address all review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717

Files:
  clangd/ClangdServer.cpp
  clangd/XRefs.cpp
  clangd/XRefs.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -13,11 +13,13 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
+#include "gmock/gmock.h"
+#include "index/SymbolCollector.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
+#include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
-#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -37,17 +39,51 @@
 };
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-  {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+StringRef BaseName = "Main",
+llvm::IntrusiveRefCntPtr
+InMemoryFileSystem = nullptr) {
+  auto HeaderPath = testPath((BaseName + ".h").str());
+  auto MainPath = testPath((BaseName + ".cpp").str());
+  if (!InMemoryFileSystem)
+InMemoryFileSystem = new vfs::InMemoryFileSystem();
+
+  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+InMemoryFileSystem->addFile(HeaderPath, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+std::vector args = {"-include", HeaderPath.c_str()};
+Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
+  }
+
+  InMemoryFileSystem->addFile(MainPath, 0,
+  llvm::MemoryBuffer::getMemBuffer(MainCode));
+
+  auto CI = createInvocationFromCommandLine(Cmd);
+
+  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
   std::make_shared(),
-  vfs::getRealFileSystem());
+  InMemoryFileSystem);
   assert(AST.hasValue());
   return std::move(*AST);
 }
 
+std::unique_ptr buildIndexFromAST(ParsedAST *AST) {
+  SymbolCollector::Options CollectorOpts;
+  CollectorOpts.CollectIncludePath = false;
+  CollectorOpts.CountReferences = false;
+  index::IndexingOptions IndexOpts;
+  IndexOpts.SystemSymbolFilter =
+  index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
+  IndexOpts.IndexFunctionLocals = false;
+  auto Collector = std::make_shared(std::move(CollectorOpts));
+  Collector->setPreprocessor(AST->getPreprocessorPtr());
+  index::indexTopLevelDecls(AST->getASTContext(), AST->getTopLevelDecls(),
+Collector, IndexOpts);
+  return MemIndex::build(Collector->takeSymbols());
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -106,6 +142,80 @@
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
+TEST(GoToDefinition, WithIndex) {
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  Annotations SymbolHeader(R"cpp(
+class $forward[[Forward]];
+class $foo[[Foo]] {};
+
+void $f1[[f1]]();
+
+inline void $f2[[f2]]() {}
+  )cpp");
+  Annotations SymbolCpp(R"cpp(
+  class $forward[[forward]] {};
+  void  $f1[[f1]]() {}
+)cpp");
+
+  // Build index.
+  auto IndexAST = build(SymbolCpp.code(), SymbolHeader.code(), "symbol",
+InMemoryFileSystem);
+  auto Index = buildIndexFromAST();
+
+  auto runFindDefinitionsWithIndex =
+  [, ](const Annotations ) {
+auto AST = build(/*MainCode=*/Main.code(),
+ /*HeaderCode=*/"",
+ /*BasePath=*/"main", InMemoryFileSystem);
+return clangd::findDefinitions(AST, Main.point(), Index.get());
+  };
+
+  Annotations Test(R"cpp(// only declaration in AST.
+void [[f1]]();
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+
+  Test = Annotations(R"cpp(// definition in AST.
+void [[f1]]() {}
+int main() {
+  ^f1();
+}
+  )cpp");
+  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray(
+  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+
+  Test = Annotations(R"cpp(// forward declaration 

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/XRefs.cpp:209
 
+  llvm::DenseSet QuerySyms;
+  llvm::DenseMap ResultCandidates;

sammccall wrote:
> the approach could some documentation - I find it hard to follow the code.
> 
> This function is probably too big by now and should be split up - but without 
> understanding the goal I can't suggest how.
Having read the code in more detail, I'd suggest a structure like:
 - we identify the symbols being searched for
 - for each, we have a definition slot and a non-definition slot
 - initially we populate one of the slots with our preferred declaration from 
the AST (if location is available)
 - then we query the index and overwrite either/both slots with returned info
 - finally, we return populated slots for all symbols, definition first

(It's not clear how to order the returned symbols if there are multiple. When 
can this happen?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/XRefs.cpp:209
 
+  llvm::DenseSet QuerySyms;
+  llvm::DenseMap ResultCandidates;

the approach could some documentation - I find it hard to follow the code.

This function is probably too big by now and should be split up - but without 
understanding the goal I can't suggest how.



Comment at: clangd/XRefs.cpp:216
+  log("Could not creat location for Loc");
+  continue;
+}

this seems suspicious to me - if we can't find an AST based location for the 
symbol, why would that mean giving up without consulting the index?



Comment at: clangd/XRefs.cpp:224
+bool IsDef = GetDefinition(D) == D;
+// Query the index if there is no definition point in the local AST.
+if (!IsDef)

isn't the condition here something else - that there's at least one declaration 
that isn't a definition?



Comment at: clangd/XRefs.cpp:239
+  assert(it != ResultCandidates.end());
+  if (Sym.Definition) {
+auto Uri = URI::parse(Sym.Definition.FileURI);

why are we ignoring the index if it's not a definition (and therefore 
preferring the AST one?)



Comment at: clangd/XRefs.cpp:261
+
+  for (auto It : ResultCandidates)
+Result.push_back(It.second);

Here we're returning exactly one candidate per symbol we identified as a 
candidate. There are other choices - why this one?



Comment at: clangd/XRefs.h:26
+std::vector findDefinitions(ParsedAST , Position Pos,
+  const SymbolIndex *const Index = 
nullptr);
 

nit: drop the second const here? we don't normally bother to mark by-value 
parameters as const (particularly not in declarations)



Comment at: unittests/clangd/XRefsTests.cpp:109
 
+TEST(GoToDefinition, WithIndex) {
+  MockFSProvider FS;

We have a findDefinitionsHelper to avoid all this boilerplate.
Can you extend/overload it it to work with index?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: MaskRay, ioeric, jkorous-apple, ilya-biryukov, klimek.

This patch adds index support for GoToDefinition -- when we don't get the
definition from local AST, we query our index (Static) index to
get it.

Since we currently collect top-level symbol in the index, it doesn't support all
cases (e.g. class members), we will extend the index to include more symbols in
the future.

Note: the newly-added test fails because of a DynamicIndex issue -- the symbol 
is
overwritten instead of merging.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717

Files:
  clangd/ClangdServer.cpp
  clangd/XRefs.cpp
  clangd/XRefs.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -106,6 +106,44 @@
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
+TEST(GoToDefinition, WithIndex) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  auto option = ClangdServer::optsForTest();
+  option.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, option);
+
+  const char *DefHeaderSource = "void ff();";
+  auto DefCpp = testPath("def.cpp");
+  auto DefHeader = testPath("def.h");
+  auto TestCpp = testPath("test.cpp");
+  Annotations Def(R"cpp(
+ #include "def.h"
+ void [[ff]]() {}
+  )cpp");
+  Annotations Test(R"cpp(
+ #include "def.h"
+ void f() {
+   ^ff();
+ }
+  )cpp");
+  FS.Files[DefCpp] = Def.code();
+  FS.Files[TestCpp] = Test.code();
+  FS.Files[DefHeader] = DefHeaderSource;
+
+  runAddDocument(Server, TestCpp, Test.code());
+  runAddDocument(Server, DefCpp, Def.code());
+  std::vector ExpectedLocations;
+  for (const auto  : Def.ranges())
+ExpectedLocations.push_back(RangeIs(R));
+
+  auto Locations = runFindDefinitions(Server, TestCpp, Test.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations, ElementsAreArray(ExpectedLocations))
+  << Test.code();
+}
+
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -14,14 +14,16 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 
 #include "ClangdUnit.h"
+#include "index/Index.h"
 #include "Protocol.h"
 #include 
 
 namespace clang {
 namespace clangd {
 
 /// Get definition of symbol at a specified \p Pos.
-std::vector findDefinitions(ParsedAST , Position Pos);
+std::vector findDefinitions(ParsedAST , Position Pos,
+  const SymbolIndex *const Index = nullptr);
 
 /// Returns highlights for all usages of a symbol at \p Pos.
 std::vector findDocumentHighlights(ParsedAST ,
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
+#include "clang/Index/USRGeneration.h"
 #include "llvm/Support/Path.h"
 namespace clang {
 namespace clangd {
@@ -128,6 +129,20 @@
   }
 };
 
+llvm::Optional
+getAbsoluteFilePath(const FileEntry *F, const SourceManager ) {
+  SmallString<64> FilePath = F->tryGetRealPathName();
+  if (FilePath.empty())
+FilePath = F->getName();
+  if (!llvm::sys::path::is_absolute(FilePath)) {
+if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+  log("Could not turn relative path to absolute: " + FilePath);
+  return llvm::None;
+}
+  }
+  return FilePath.str().str();
+}
+
 llvm::Optional
 makeLocation(ParsedAST , const SourceRange ) {
   const SourceManager  = AST.getASTContext().getSourceManager();
@@ -145,24 +160,19 @@
   Range R = {Begin, End};
   Location L;
 
-  SmallString<64> FilePath = F->tryGetRealPathName();
-  if (FilePath.empty())
-FilePath = F->getName();
-  if (!llvm::sys::path::is_absolute(FilePath)) {
-if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
-  log("Could not turn relative path to absolute: " + FilePath);
-  return llvm::None;
-}
-  }
-
-  L.uri = URIForFile(FilePath.str());
+  auto FilePath = getAbsoluteFilePath(F, SourceMgr);
+  if (!FilePath)
+return llvm::None;
+  L.uri = URIForFile(*FilePath);
   L.range = R;
   return L;
 }
 
 } // namespace
 
-std::vector findDefinitions(ParsedAST , Position Pos) {
+std::vector
+findDefinitions(ParsedAST , Position Pos,
+const SymbolIndex *const Index) {
   const SourceManager  = AST.getASTContext().getSourceManager();
   const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
   if (!FE)
@@ -196,13 +206,61 @@
   std::vector Decls = DeclMacrosFinder->takeDecls();