[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325395: [clangd] Implement textDocument/hover (authored by 
malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D35894

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/XRefs.h
  clang-tools-extra/trunk/test/clangd/hover.test
  clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
  clang-tools-extra/trunk/test/clangd/initialize-params.test
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clang-tools-extra/trunk/test/clangd/hover.test
===
--- clang-tools-extra/trunk/test/clangd/hover.test
+++ clang-tools-extra/trunk/test/clangd/hover.test
@@ -0,0 +1,19 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":27}}}
+#  CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"contents": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+ 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE325395: [clangd] Implement textDocument/hover (authored by 
malaperle, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D35894?vs=134708=134712#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- test/clangd/hover.test
+++ test/clangd/hover.test
@@ -0,0 +1,19 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":27}}}
+#  CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"contents": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

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

Works well and code looks good. There were only minor tweaks since the last 
approval. I'll land this since there are some issues with Simon's svn account.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134708.
malaperle added a comment.

Add a missing std::move, fix some formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134692.
malaperle added a comment.

Rebase.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;
+}
+  )cpp",
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134452.
simark added a comment.

Fix field names in XRefsTests.cpp

I realized I forgot to add some fixups in the previous version, the field names
in XRefsTests.cpp were not updated and therefore it did not build.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

Thanks for fixing all the NITs!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134440.
simark added a comment.

Fix some nits

- Revert case change of struct fields in Protocol.h
- Change return formatting in onHover
- Use flush() + return Name in NamedDeclQualifiedName and TypeDeclToString


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+ 

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1008861, @ilya-biryukov wrote:

> Only the naming of fields left.
>
> Also, please make sure to run `git-clang-format` on the code before 
> submitting to make sure it's formatted properly.


Ahh, I was running clang-format by hand, didn't know about git-clang-format 
(and obviously forgot some files). Thanks.




Comment at: clangd/ClangdLSPServer.cpp:331
+ if (!H) {
+   replyError(ErrorCode::InternalError,
+  llvm::toString(H.takeError()));

ilya-biryukov wrote:
> NIT: use `return replyError` to be consistent with other methods.
Ok.



Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > We should call `flush()` before returning `Name` here. The 
> > > `raw_string_ostream` is buffered.
> > I replaced it with `Stream.str()`.
> Also works, albeit less efficient (will probably do a copy where move is 
> enough).
Ah, didn't think about that.  I'll change it to flush + return Name.



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > NIT: LLVM uses `UpperCamelCase` for field names.
> > Ok, I fixed all the structs this patch adds.
> Argh, sorry that I didn't mention it before, but we have an exception for 
> structs in `Procotol.h`, they follow the spelling in the LSP spec (i.e. 
> should be `lowerCamelCase`).
> 
Haha, no problem, changing them back is one keystroke away.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Only the naming of fields left.

Also, please make sure to run `git-clang-format` on the code before submitting 
to make sure it's formatted properly.




Comment at: clangd/ClangdLSPServer.cpp:331
+ if (!H) {
+   replyError(ErrorCode::InternalError,
+  llvm::toString(H.takeError()));

NIT: use `return replyError` to be consistent with other methods.



Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

simark wrote:
> ilya-biryukov wrote:
> > We should call `flush()` before returning `Name` here. The 
> > `raw_string_ostream` is buffered.
> I replaced it with `Stream.str()`.
Also works, albeit less efficient (will probably do a copy where move is 
enough).



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

simark wrote:
> ilya-biryukov wrote:
> > NIT: LLVM uses `UpperCamelCase` for field names.
> Ok, I fixed all the structs this patch adds.
Argh, sorry that I didn't mention it before, but we have an exception for 
structs in `Procotol.h`, they follow the spelling in the LSP spec (i.e. should 
be `lowerCamelCase`).



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1008593, @ilya-biryukov wrote:

> Just a few last remarks and this is good to go.
>  Should I land it for you after the last comments are fixed?


I just received my commit access, so if you are ok with it, I would try to push 
it once I get your final OK.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134437.
simark added a comment.

New version

- Rebase on master, change findHover to have a callback-based interface
- Fix nits from previous review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

ilya-biryukov wrote:
> We should call `flush()` before returning `Name` here. The 
> `raw_string_ostream` is buffered.
I replaced it with `Stream.str()`.



Comment at: clangd/XRefs.cpp:373
+
+  return {};
+}

ilya-biryukov wrote:
> NIT: use `llvm::None` here instead of `{}`
Done.



Comment at: clangd/XRefs.cpp:394
+
+  //  SourceRange SR = D->getSourceRange();
+

ilya-biryukov wrote:
> Accidental leftover from previous code?
Oops, thanks.



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

ilya-biryukov wrote:
> NIT: LLVM uses `UpperCamelCase` for field names.
Ok, I fixed all the structs this patch adds.



Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }

ilya-biryukov wrote:
> simark wrote:
> > Note that I used `.str()` here to make the output of failing tests readable 
> > and useful.  By default, gtest tries to print StringRef as if it was a 
> > container.  I tried to make add a `PrintTo` function to specify how it 
> > should be printed, but couldn't get it to work (to have it called by the 
> > compiler), so I settled for this.
> Thanks for spotting that.
> We have a fix for that in LLVM's gtest extensions (D43330).
> `str()` can now be removed.
Removed.  Even if this patch is merged before D43330 it's not a big deal, since 
it only affects debug output of failing test cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Just a few last remarks and this is good to go.
Should I land it for you after the last comments are fixed?




Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

We should call `flush()` before returning `Name` here. The `raw_string_ostream` 
is buffered.



Comment at: clangd/XRefs.cpp:373
+
+  return {};
+}

NIT: use `llvm::None` here instead of `{}`



Comment at: clangd/XRefs.cpp:394
+
+  //  SourceRange SR = D->getSourceRange();
+

Accidental leftover from previous code?



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

NIT: LLVM uses `UpperCamelCase` for field names.



Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }

simark wrote:
> Note that I used `.str()` here to make the output of failing tests readable 
> and useful.  By default, gtest tries to print StringRef as if it was a 
> container.  I tried to make add a `PrintTo` function to specify how it should 
> be printed, but couldn't get it to work (to have it called by the compiler), 
> so I settled for this.
Thanks for spotting that.
We have a fix for that in LLVM's gtest extensions (D43330).
`str()` can now be removed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }

Note that I used `.str()` here to make the output of failing tests readable and 
useful.  By default, gtest tries to print StringRef as if it was a container.  
I tried to make add a `PrintTo` function to specify how it should be printed, 
but couldn't get it to work (to have it called by the compiler), so I settled 
for this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134306.
simark added a comment.

Generate Hover by pretty-printing the AST

This new version of the patch generates the hover by pretty-printing the AST.
The unit tests are updated accordingly.  There are some inconsistencies in how
things are printed.  For example, I find it a bit annoying that we print empty
curly braces after class names (e.g. "class Foo {}").  Also, namespace and
enums are printed with a newline between the two braces.  These are things that
could get fixed by doing changes to the clang AST printer.

The hover.test test now uses a more readable format.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#1006124, @simark wrote:

> Is there a way to get the macro name from the MacroInfo object?  I couldn't 
> find any, so the solution I'm considering is to make 
> `DeclarationAndMacrosFinder::takeMacroInfos` return an 
> `std::vector>`, where the first 
> member of the pair is the macro name.  It would come from 
> `IdentifierInfo->getName()` in `DeclarationAndMacrosFinder::finish`.  Does 
> that make sense, or is there a simpler way?


I don't think there's a way to get macro name from `MacroInfo`. `pair` sounds good to me, I'd probably even use a named struct here: 
`struct MacroDecl { StringRef Name; const MacroInfo  }`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

> I'm not aware of the code that pretty-prints macros. There's a code that 
> dumps debug output, but it's not gonna help in that case.
>  Alternatively, we could simply print the macro name and not print the 
> definition. That's super-simple and is in line with hovers for the AST decls. 
> Let's do that in the initial version.

Ok, I'm considering printing "#define MACRO".  Just printing the name would not 
be very useful, at least printing "#define" gives the information that the 
hovered entity is defined as a macro (and not a function or variable).

Is there a way to get the macro name from the MacroInfo object?  I couldn't 
find any, so the solution I'm considering is to make 
`DeclarationAndMacrosFinder::takeMacroInfos` return an 
`std::vector>`, where the first member 
of the pair is the macro name.  It would come from `IdentifierInfo->getName()` 
in `DeclarationAndMacrosFinder::finish`.  Does that make sense, or is there a 
simpler way?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#1003356, @simark wrote:

> In https://reviews.llvm.org/D35894#1003342, @simark wrote:
>
> > The only problem I see is that when hovering a function/struct name, it now 
> > prints the whole function/struct definition.  When talking with @malaperle, 
> > he told me that you had discussed it before and we should not have the 
> > definition in the hover, just the prototype for a function and the name 
> > (e.g. "struct foo") for a struct.  Glancing quickly at the PrintingPolicy, 
> > I don't see a way to do that.  Do you have any idea?
>
>
> Arrg, Sorry, I just re-read your comment and saw that you used TerseOutput 
> which does that.  I did not catch that when transcribing the code (I did not 
> want to copy paste to understand the code better, but got bitten).


Yeah, the `TerseOutput` bit was important :-)

In https://reviews.llvm.org/D35894#1003342, @simark wrote:

> The only problem I see is that when hovering a function/struct name, it now 
> prints the whole function/struct definition.  When talking with @malaperle, 
> he told me that you had discussed it before and we should not have the 
> definition in the hover, just the prototype for a function and the name (e.g. 
> "struct foo") for a struct.  Glancing quickly at the PrintingPolicy, I don't 
> see a way to do that.  Do you have any idea?


I'm not aware of the code that pretty-prints macros. There's a code that dumps 
debug output, but it's not gonna help in that case.
Alternatively, we could simply print the macro name and not print the 
definition. That's super-simple and is in line with hovers for the AST decls. 
Let's do that in the initial version.

For macros using the source code is also less of a problem, so we may try to do 
it. C++ doesn't allow to generate a macro inside another macro, so we don't run 
into the same problems we may hit with the AST. And the overall syntax is much 
simpler than for AST, so we can hope to get string manipulation right with 
reasonable effort. (Simply reindenting should do the trick).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1003342, @simark wrote:

> The only problem I see is that when hovering a function/struct name, it now 
> prints the whole function/struct definition.  When talking with @malaperle, 
> he told me that you had discussed it before and we should not have the 
> definition in the hover, just the prototype for a function and the name (e.g. 
> "struct foo") for a struct.  Glancing quickly at the PrintingPolicy, I don't 
> see a way to do that.  Do you have any idea?


Arrg, Sorry, I just re-read your comment and saw that you used TerseOutput 
which does that.  I did not catch that when transcribing the code (I did not 
want to copy paste to understand the code better, but got bitten).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

The only problem I see is that when hovering a function name, it now prints the 
whole function definition.  When talking with @malaperle, he told me that you 
had discussed it before and we should not have the definition in the hover, 
just the prototype.  Glancing quickly at the PrintingPolicy, I don't see a way 
to just print the prototype of the function.  Do you have any idea?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Another example where pretty-printing the AST gives better results:

  int x, y = 5, z = 6;

Hover the `z` will now show `int z = 6`, before it would have shown `int x, y = 
5, z = 6`.  I think new version is better because it only shows the variable we 
care about.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:231
+TEST(Hover, All) {
+
+  struct OneTest {

ilya-biryukov wrote:
> NIT: remove empty line at the start of the function
Done.



Comment at: unittests/clangd/XRefsTests.cpp:233
+  struct OneTest {
+const char *input;
+const char *expectedHover;

ilya-biryukov wrote:
> NIT: Use `StringRef` instead of `const char*`
Done.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:325
+void ClangdLSPServer::onHover(TextDocumentPositionParams ) {
+
+  llvm::Expected H =

ilya-biryukov wrote:
> NIT: remove the empty line at the start of function
Done.



Comment at: clangd/Protocol.cpp:324
+json::Expr toJSON(const MarkupContent ) {
+  const char *KindStr = NULL;
+

ilya-biryukov wrote:
> NIT: use nullptr instead of NULL
> (irrelevant if we use `StringRef`, see other comment below)
Ack.



Comment at: clangd/Protocol.cpp:331
+
+  switch (MC.kind) {
+  case MarkupKind::PlainText:

ilya-biryukov wrote:
> - `StringRef` is a nicer abstraction, maybe use it instead of `const char*`?
> - Maybe move declaration of `KindStr` closer to its usage?
> - Maybe extract a helper function to mark the code path for invalid kinds as 
> unreachable? I.e.
> ```
> static StringRef toTextKind(MarkupKind Kind) {
>   switch (Kind) {
> case MarkupKind::PlainText:
>   return "plaintext";
> case MarkupKind::Markdown:
>   return "markdown";
>   }
>   llvm_unreachable("Invalid MarkupKind");
> }
> ```
Done.



Comment at: clangd/XRefs.cpp:326
+  if (const TagDecl *TD = dyn_cast(ND)) {
+switch (TD->getTagKind()) {
+case TTK_Class:

ilya-biryukov wrote:
> Same suggestion as before. Could we extract a helper function to mark invalid 
> enum values unreachable?
> 
Done.



Comment at: clangd/XRefs.cpp:552
+  if (!Decls.empty()) {
+assert(Decls[0] != nullptr);
+if (Decls[0] != nullptr)

ilya-biryukov wrote:
> This assert seems rather unlikely and just makes the code more complex. Let's 
> assume it's true and remove it (along with the subsequent if statement)
Done...



Comment at: clangd/XRefs.cpp:560
+assert(Macros[0] != nullptr);
+if (Macros[0] != nullptr)
+  return getHoverContents(Macros[0], AST);

ilya-biryukov wrote:
> This assert seems rather unlikely and just makes the code more complex. Let's 
> assume it's true and remove it (along with the subsequent if statement)
> 
> 
... and done.



Comment at: test/clangd/hover.test:2
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#

ilya-biryukov wrote:
> We have a more readable format for the lit-tests now. Could we update this 
> test to use it?
> (see other tests in test/clangd for example)
Indeed the new format looks much more readable.  I'll modify this one to look 
like completion.test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1001875, @ilya-biryukov wrote:

> Thanks for picking this up!
>  I have just one major comment related to how we generate the hover text.
>
> Current implementation tries to get the actual source code for every 
> declaration and then patch it up to look nice on in the output.
>  It is quite a large piece of code (spanning most of the patch) and tries to 
> mix and match source code and output from the AST when there's not enough 
> information in the AST (e.g. `tryFixupIndentation` and 
> `stripOpeningBracket`). This approach is really complex as we'll inevitably 
> try to parse "parts" of C++ and figure out how to deal with macros, etc.
>  Worse than that, I would expect this code to grow uncontrollably with time 
> and be very hard to maintain.
>  Moreover, in presence of macros it arguably gives non-useful results. For 
> example, consider this:
>
>   #define DEFINITIONS int foo(); int bar(); int baz();
>  
>   // somewhere else
>   DEFINITIONS
>  
>   // somewhere else
>   int test() {
> foo(); // <-- hover currently doesn't work here. And even if it did, 
> showing a line with just DEFINITIONS is not that useful.
>   }
>
>
> I suggest we move to a different approach of pretty-printing the relevant 
> parts of the AST. It is already implemented in clang, handles all the cases 
> in the AST (and will be updated along when AST is changed), shows useful 
> information in presence of macros and is much easier to implement.
>  The version of `getHoverContents` using this is just a few lines of code:
>
>   static std::string getHoverContents(const Decl *D) {
> if (TemplateDecl *TD = D->getDescribedTemplate())
>   D = TD; // we want to see the template bits.
>  
> std::string Result;
> llvm::raw_string_ostream OS(Result);
>  
> PrintingPolicy Policy(D->getASTContext().getLangOpts());
> Policy.TerseOutput = true;
> D->print(OS, Policy);
>  
> OS.flush();
> return Result;
>   }
>
>
> It doesn't add the `"Defined in ..."` piece, but illustrates the APIs we can 
> use. We should use the same APIs for the scope as well, avoiding fallbacks to 
> manual printing if we can.
>  If there's something missing/wrong in the upstream pretty printers, it's 
> fine and we can fix them (e.g., the thing that I've immediately noticed is 
> that namespaces are always printed with curly braces).


I thought it would be nice to show in the hover the declaration formatted as it 
is written in the source file, because that's how the developer is used to read 
it.  But on the other hand, I completely agree that trying to do string 
formatting ourselves is opening a can of worms.  I'll try the approach you 
suggest.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for picking this up!
I have just one major comment related to how we generate the hover text.

Current implementation tries to get the actual source code for every 
declaration and then patch it up to look nice on in the output.
It is quite a large piece of code (spanning most of the patch) and tries to mix 
and match source code and output from the AST when there's not enough 
information in the AST (e.g. `tryFixupIndentation` and `stripOpeningBracket`). 
This approach is really complex as we'll inevitably try to parse "parts" of C++ 
and figure out how to deal with macros, etc.
Worse than that, I would expect this code to grow uncontrollably with time and 
be very hard to maintain.
Moreover, in presence of macros it arguably gives non-useful results. For 
example, consider this:

  #define DEFINITIONS int foo(); int bar(); int baz();
  
  // somewhere else
  DEFINITIONS
  
  // somewhere else
  int test() {
foo(); // <-- hover currently doesn't work here. And even if it did, 
showing a line with just DEFINITIONS is not that useful.
  }

I suggest we move to a different approach of pretty-printing the relevant parts 
of the AST. It is already implemented in clang, handles all the cases in the 
AST (and will be updated along when AST is changed), shows useful information 
in presence of macros and is much easier to implement.
The version of `getHoverContents` using this is just a few lines of code:

  static std::string getHoverContents(const Decl *D) {
if (TemplateDecl *TD = D->getDescribedTemplate())
  D = TD; // we want to see the template bits.
  
std::string Result;
llvm::raw_string_ostream OS(Result);
  
PrintingPolicy Policy(D->getASTContext().getLangOpts());
Policy.TerseOutput = true;
D->print(OS, Policy);
  
OS.flush();
return Result;
  }

It doesn't add the `"Defined in ..."` piece, but illustrates the APIs we can 
use. We should use the same APIs for the scope as well, avoiding fallbacks to 
manual printing if we can.
If there's something missing/wrong in the upstream pretty printers, it's fine 
and we can fix them (e.g., the thing that I've immediately noticed is that 
namespaces are always printed with curly braces).




Comment at: clangd/ClangdLSPServer.cpp:325
+void ClangdLSPServer::onHover(TextDocumentPositionParams ) {
+
+  llvm::Expected H =

NIT: remove the empty line at the start of function



Comment at: clangd/Protocol.cpp:324
+json::Expr toJSON(const MarkupContent ) {
+  const char *KindStr = NULL;
+

NIT: use nullptr instead of NULL
(irrelevant if we use `StringRef`, see other comment below)



Comment at: clangd/Protocol.cpp:331
+
+  switch (MC.kind) {
+  case MarkupKind::PlainText:

- `StringRef` is a nicer abstraction, maybe use it instead of `const char*`?
- Maybe move declaration of `KindStr` closer to its usage?
- Maybe extract a helper function to mark the code path for invalid kinds as 
unreachable? I.e.
```
static StringRef toTextKind(MarkupKind Kind) {
  switch (Kind) {
case MarkupKind::PlainText:
  return "plaintext";
case MarkupKind::Markdown:
  return "markdown";
  }
  llvm_unreachable("Invalid MarkupKind");
}
```



Comment at: clangd/XRefs.cpp:326
+  if (const TagDecl *TD = dyn_cast(ND)) {
+switch (TD->getTagKind()) {
+case TTK_Class:

Same suggestion as before. Could we extract a helper function to mark invalid 
enum values unreachable?




Comment at: clangd/XRefs.cpp:552
+  if (!Decls.empty()) {
+assert(Decls[0] != nullptr);
+if (Decls[0] != nullptr)

This assert seems rather unlikely and just makes the code more complex. Let's 
assume it's true and remove it (along with the subsequent if statement)



Comment at: clangd/XRefs.cpp:560
+assert(Macros[0] != nullptr);
+if (Macros[0] != nullptr)
+  return getHoverContents(Macros[0], AST);

This assert seems rather unlikely and just makes the code more complex. Let's 
assume it's true and remove it (along with the subsequent if statement)





Comment at: test/clangd/hover.test:2
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#

We have a more readable format for the lit-tests now. Could we update this test 
to use it?
(see other tests in test/clangd for example)



Comment at: unittests/clangd/XRefsTests.cpp:231
+TEST(Hover, All) {
+
+  struct OneTest {

NIT: remove empty line at the start of the function



Comment at: unittests/clangd/XRefsTests.cpp:233
+  struct OneTest {
+const char *input;
+const char *expectedHover;

NIT: Use `StringRef` instead of `const char*`


Repository:
  rCTE Clang Tools Extra

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 133367.
simark added a comment.
Herald added subscribers: ioeric, jkorous-apple.

New version

Here's a new version of the patch, quite reworked.  I scaled down the scope a
little bit to make it simpler (see commit log).  We'll add more features later
on.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -227,6 +227,316 @@
   }
 }
 
+TEST(Hover, All) {
+
+  struct OneTest {
+const char *input;
+const char *expectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;
+}
+  )cpp",
+  "Declared in global namespace\n\ntypedef int Foo",
+  },
+  {
+  R"cpp(// Namespace
+namespace ns {
+struct Foo { static 

[PATCH] D35894: [clangd] Code hover for Clangd

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/ClangdUnit.cpp:941
   createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine, 
VFS);
+  CI->LangOpts->CommentOpts.ParseAllComments = true;
   // createInvocationFromCommandLine sets DisableFree.

Doesn't this slow down parsing by a lot? I thought @ilya-biryukov had seen that 
it was responsible for big performance losses in code complete.

ISTM we might prefer to look up the comment from the index by USR, and just 
live without it if we can't find it.

(Apologies if this has been discussed)




Comment at: clangd/ClangdUnit.h:1
 //===--- ClangdUnit.h ---*- C++-*-===//
 //

Please don't cram more stuff into clangdunit that's not directly related to 
building ASTs and managing their lifetimes. Instead, can we pull as much as 
possible out into `Documentation.h` or similar?

Note not `Hover.h` as there are other use cases, like we should use this logic 
for the code completion item documentation too.



Comment at: test/clangd/hover.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.

Could I ask you to do primary/exhaustive testing of this with unit tests, and 
just a basic smoke test with lit?
Then it's possible to read the tests, understand whether they're correct, and 
understand the failure messages.

I'm trying to pull more of the features in this direction - see 
`CodeCompleteTests.cpp` for an example of how this can work.

We've had quite a few cases of bugs that had tests, but nobody noticed that the 
tests were wrong because it's just not possible to read them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 127413.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.

  findHover properly returns an error
  Removed many cases of bad merging
  Separated getHover in two functions
  Minor indenting and NIT fixes


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -31,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -31,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,56 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 611
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\n//comments test\ntemplate\nT templateTest(T foo) {\nreturn foo;}\ntemplate\nclass classTemplateTest {\npublic: T test;};\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;\nint temp = 5;\ntemplateTest(temp);classTemplateTest test;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":20},"start":{"character":0,"line":20
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":22,"character":15}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":23,"character":10}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":24,"character":13}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":25,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.



Comment at: clangd/ClangdServer.h:306
+  /// Run formatting for \p Rng inside \p File.
+  std::vector formatRange(PathRef File, Range Rng);
+  /// Run formatting for the whole \p File.

ilya-biryukov wrote:
> These functions are duplicates of existing ones, they should not be here.
> Bad merge?
Yes, this is a bad merge.



Comment at: clangd/ClangdUnit.cpp:438
+  // trimming the extra "::"
+  std::string Res;
+  PrintingPolicy WithScopePP(LangOpts);

ilya-biryukov wrote:
> We should avoid doing the string-matching when we have the AST.
> Use something like this instead (haven't tested, but you should get the idea):
> 
> ```
> /// Returns a NamedDecl that could be used to print the qualifier.
> Decl* getScopeOfDecl(Decl* D) {
>   // Code from NamedDecl::printQualifiedName, we should probably move it into 
>   const DeclContext *Ctx = D->getDeclContext();
> 
>   // For ObjC methods, look through categories and use the interface as 
> context.
>   if (auto *MD = dyn_cast(D))
> if (auto *ID = MD->getClassInterface())
>   Ctx = ID;
> 
>   if (Ctx->isFunctionOrMethod()) {
> /// This is a function-local entity.
> return nullptr;
>   }
> 
>   return Ctx;
> }
> 
> 
> std::string printScopeOfDecl(Decl* Scope) {
>   if (isa(Scope))
> return "global namespace";
> 
>   if (isa(Scope))
> return cast(Scope)->printQualifiedName();
> 
>   return "";
> }
> 
> 
> // Later use like this:
> auto ScopeText = printScopeOfDecl(getScopeOfDecl(D));
> ```
> 
I'm not sure I understand. The intention was to have a separate MarkedString 
display the scope for whatever we are hovering on. The way I understand the 
above code, this would display a scope that isn't as precise as I would like 
which defeats the purpose of having this feature in the first place.



Comment at: clangd/ClangdUnit.cpp:498
   Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+  Begin.line = SourceMgr.getExpansionLineNumber(LocStart) - 1;
+  Begin.character = SourceMgr.getExpansionColumnNumber(LocStart) - 1;

ilya-biryukov wrote:
> Why are we changing the semantics here? Why should we use expansion locations 
> instead of spelling locations here?
Bad merge.



Comment at: clangd/ClangdUnit.cpp:591
+// one line at a time to determine what will be included.
+SourceLocation getFunctionComments(ParsedAST , const Decl *D) {
+

ilya-biryukov wrote:
> This code seems pretty involved. Don't `RawComment`s handle this case?
Bad merge.



Comment at: clangd/ClangdUnit.cpp:968
   // createInvocationFromCommandLine sets DisableFree.
+  CI->LangOpts->CommentOpts.ParseAllComments = true;
   CI->getFrontendOpts().DisableFree = false;

ilya-biryukov wrote:
> This should clearly go before the comment!
> Also, we're currently setting this flag when building the preamble, but not 
> when parsing the AST. We should definitely do that in both cases.
Do you mean also adding this line somewhere in ASTUnit.cpp?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:284
+void ClangdLSPServer::onCodeHover(Ctx C, TextDocumentPositionParams ) {
+
+  auto H = Server.findHover(C,

NIT: remove empty line



Comment at: clangd/ClangdServer.cpp:428
   std::vector Result;
-  Resources->getAST().get()->runUnderLock([Pos, , ](ParsedAST *AST) 
{
+  Resources->getAST().get()->runUnderLock([Pos, , , this](ParsedAST 
*AST) {
 if (!AST)

We don't need to capture `this`, remove it from the capture list.



Comment at: clangd/ClangdServer.cpp:550
   std::shared_ptr Resources = Units.getFile(File);
-  if (!Resources)
-return llvm::make_error(
-"findDocumentHighlights called on non-added file",
-llvm::errc::invalid_argument);
-
-  std::vector Result;
-  llvm::Optional Err;
-  Resources->getAST().get()->runUnderLock([Pos, , ,
-   ](ParsedAST *AST) {
-if (!AST) {
-  Err = llvm::make_error("Invalid AST",
-llvm::errc::invalid_argument);
-  return;
-}
-Result = clangd::findDocumentHighlights(Ctx, *AST, Pos);
+  assert(Resources && "Calling findHover on non-added file");
+  Resources->getAST().get()->runUnderLock(

Return an error instead of asserting to follow how other functions do that:
```
   if (!Resources)
 return llvm::make_error(
 "findHover called on non-added file",
 llvm::errc::invalid_argument);
```



Comment at: clangd/ClangdServer.h:303
 
+  llvm::Expected findHover(const Context , PathRef File, 
Position Pos);
+

Please put this right after `findDocumentHighlights`. It is really weird to see 
`findHover` in the middle of formatting functions.



Comment at: clangd/ClangdServer.h:306
+  /// Run formatting for \p Rng inside \p File.
+  std::vector formatRange(PathRef File, Range Rng);
+  /// Run formatting for the whole \p File.

These functions are duplicates of existing ones, they should not be here.
Bad merge?



Comment at: clangd/ClangdUnit.cpp:222
   return Future.wait_for(std::chrono::seconds(0)) == std::future_status::ready;
+
 }

NIT: remove the empty line



Comment at: clangd/ClangdUnit.cpp:288
+/// Finds declarations and macros that a given source location refers to.
+
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {

NIT: remove the empty line



Comment at: clangd/ClangdUnit.cpp:303
   std::vector takeDecls() {
-// Don't keep the same declaration multiple times.
+// Don't keep the same location multiple times.
 // This can happen when nodes in the AST are visited twice.

Bad merge? This comment should not be changed.



Comment at: clangd/ClangdUnit.cpp:308
 Decls.erase(Last, Decls.end());
-return std::move(Decls);
+return Decls;
   }

`std::move(Decls)` is better. Bad merge?



Comment at: clangd/ClangdUnit.cpp:313
 // Don't keep the same Macro info multiple times.
+// This can happen when nodes in the AST are visited twice.
 std::sort(MacroInfos.begin(), MacroInfos.end());

This comment is not upstream now and we're removed it in the other patch. 
Remove it here too.



Comment at: clangd/ClangdUnit.cpp:438
+  // trimming the extra "::"
+  std::string Res;
+  PrintingPolicy WithScopePP(LangOpts);

We should avoid doing the string-matching when we have the AST.
Use something like this instead (haven't tested, but you should get the idea):

```
/// Returns a NamedDecl that could be used to print the qualifier.
Decl* getScopeOfDecl(Decl* D) {
  // Code from NamedDecl::printQualifiedName, we should probably move it into 
  const DeclContext *Ctx = D->getDeclContext();

  // For ObjC methods, look through categories and use the interface as context.
  if (auto *MD = dyn_cast(D))
if (auto *ID = MD->getClassInterface())
  Ctx = ID;

  if (Ctx->isFunctionOrMethod()) {
/// This is a function-local entity.
return nullptr;
  }

  return Ctx;
}


std::string printScopeOfDecl(Decl* Scope) {
  if (isa(Scope))
return "global namespace";

  if (isa(Scope))
return cast(Scope)->printQualifiedName();

  return "";
}


// Later use like this:
auto ScopeText = printScopeOfDecl(getScopeOfDecl(D));
```




Comment at: clangd/ClangdUnit.cpp:462
+/// Returns the file buffer data that the given SourceRange points to.
+llvm::ErrorOr getBufferDataFromSourceRange(ParsedAST ,
+  Location L) {

NIT: `llvm::Expected` instead of `llvm::ErrorOr`



Comment at: clangd/ClangdUnit.cpp:485
 
-llvm::Optional
+llvm::ErrorOr
 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 127131.
Nebiroth added a comment.

Rebase on master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -31,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -31,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,56 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 611
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\n//comments test\ntemplate\nT templateTest(T foo) {\nreturn foo;}\ntemplate\nclass classTemplateTest {\npublic: T test;};\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;\nint temp = 5;\ntemplateTest(temp);classTemplateTest test;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":20},"start":{"character":0,"line":20
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":22,"character":15}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":23,"character":10}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":24,"character":13}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":25,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":26,"character":8}}}
+# CHECK: 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: mgrang.

Needs to be rebased.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle accepted this revision.
malaperle added a comment.

This looks good to me. @ilya-biryukov Any objections?
I think we can do further iterations later to handle macros and other specific 
cases (multiple Decls at the position, etc) . It's already very useful 
functionality.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 125224.
Nebiroth added a comment.

Minor code cleanup
Merge with master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,56 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 611
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\n//comments test\ntemplate\nT templateTest(T foo) {\nreturn foo;}\ntemplate\nclass classTemplateTest {\npublic: T test;};\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;\nint temp = 5;\ntemplateTest(temp);classTemplateTest test;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":20},"start":{"character":0,"line":20
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":22,"character":15}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":23,"character":10}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":24,"character":13}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":25,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":26,"character":8}}}
+# CHECK: 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.



Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
-#include 
+#include "llvm/Support/YAMLParser.h"
 #include 

malaperle wrote:
> Nebiroth wrote:
> > malaperle wrote:
> > > revert this change?
> > #include  is not needed.
> I meant removing YAMLParser.h
That's what I figured. I'll remove that.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
-#include 
+#include "llvm/Support/YAMLParser.h"
 #include 

Nebiroth wrote:
> malaperle wrote:
> > revert this change?
> #include  is not needed.
I meant removing YAMLParser.h


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 9 inline comments as done.
Nebiroth added inline comments.



Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
-#include 
+#include "llvm/Support/YAMLParser.h"
 #include 

malaperle wrote:
> revert this change?
#include  is not needed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 124945.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.

  Simplified getHover() function
  Proper usage of ErrorOr to return errors
  Given range for Hover struct now only applies to the open file
  Fixed crash on MacroExpansion


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,56 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 611
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\n//comments test\ntemplate\nT templateTest(T foo) {\nreturn foo;}\ntemplate\nclass classTemplateTest {\npublic: T test;};\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;\nint temp = 5;\ntemplateTest(temp);classTemplateTest test;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":20},"start":{"character":0,"line":20
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":22,"character":15}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":23,"character":10}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":24,"character":13}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":25,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1117
+
+  if (!DeclVector.empty()) {
+const Decl *LocationDecl = DeclVector[0];

malaperle wrote:
> malaperle wrote:
> > malaperle wrote:
> > > I think we need to simplify this part a bit. I'll try to find a way. Feel 
> > > free to wait until more comments before updating.
> > Here's the version in which I tried to simplify this *a bit*. With the new 
> > ErrorOr checks as well.
> > 
> > ```
> > Hover clangd::getHover(ParsedAST , Position Pos, clangd::Logger 
> > ) {
> >   const SourceManager  = AST.getASTContext().getSourceManager();
> >   const LangOptions  = AST.getASTContext().getLangOpts();
> >   const FileEntry *FE = 
> > SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
> >   if (!FE)
> > return Hover();
> > 
> >   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
> > 
> >   auto DeclMacrosFinder = std::make_shared(
> >   llvm::errs(), SourceLocationBeg, AST.getASTContext(),
> >   AST.getPreprocessor());
> >   index::IndexingOptions IndexOpts;
> >   IndexOpts.SystemSymbolFilter =
> >   index::IndexingOptions::SystemSymbolFilterKind::All;
> >   IndexOpts.IndexFunctionLocals = true;
> >   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
> >  DeclMacrosFinder, IndexOpts);
> > 
> >   auto Decls = DeclMacrosFinder->takeDecls();
> >   if (!Decls.empty() && Decls[0]) {
> > const Decl *LocationDecl = Decls[0];
> > std::vector HoverContents;
> > 
> > // Compute scope as the first "section" of the hover.
> > if (const NamedDecl *NamedD = dyn_cast(LocationDecl)) {
> >   std::string Scope = getScope(NamedD, 
> > AST.getASTContext().getLangOpts());
> >   if (!Scope.empty()) {
> > MarkedString Info = {"", "C++", "In " + Scope};
> > HoverContents.push_back(Info);
> >   }
> > }
> > 
> > // Adjust beginning of hover text depending on the presence of 
> > templates and comments.
> > TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
> > const Decl * BeginDecl = TDec ? TDec : LocationDecl;
> > SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
> > RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
> > BeginDecl);
> > if (RC)
> >   BeginLocation = RC->getLocStart();
> > 
> > // Adjust end of hover text for things that have braces/bodies. We don't
> > // want to show the whole definition of a function, class, etc.
> > SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
> > if (auto FuncDecl = dyn_cast(LocationDecl)) {
> >   if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
> > EndLocation = FuncDecl->getBody()->getLocStart();
> > } else if (auto TagDeclaration = dyn_cast(LocationDecl)) {
> >   if (TagDeclaration->isThisDeclarationADefinition())
> > EndLocation = TagDeclaration->getBraceRange().getBegin();
> > } else if (auto NameDecl = dyn_cast(LocationDecl)) {
> >   SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
> >   LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
> >   if (BeforeLBraceLoc.isValid())
> > EndLocation = BeforeLBraceLoc;
> > }
> > 
> > SourceRange SR(BeginLocation, EndLocation);
> > if (SR.isValid()) {
> >   auto L = getDeclarationLocation(AST, SR);
> >   if (L) {
> > auto Ref = getBufferDataFromSourceRange(AST, *L);
> > if (Ref) {
> >   Hover H;
> >   if (SourceMgr.isInMainFile(BeginLocation))
> > H.range = L->range;
> >   MarkedString MS = {"", "C++", *Ref};
> >   HoverContents.push_back(MS);
> >   H.contents = std::move(HoverContents);
> >   return H;
> > }
> >   }
> > }
> >   }
> > 
> >   auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
> >   if (!MacroInfos.empty() && MacroInfos[0]) {
> > const MacroInfo *MacroInf = MacroInfos[0];
> > SourceRange SR(MacroInf->getDefinitionLoc(),
> >  MacroInf->getDefinitionEndLoc());
> > if (SR.isValid()) {
> >   auto L = getDeclarationLocation(AST, SR);
> >   if (L) {
> > auto Ref = getBufferDataFromSourceRange(AST, *L);
> > if (Ref) {
> >   Hover H;
> >   if (SourceMgr.isInMainFile(SR.getBegin()))
> > H.range = L->range;
> >   MarkedString MS = {"", "C++", "#define " + Ref->str()};
> >   H.contents.push_back(MS);
> >   return H;
> > }
> >   }
> > }
> >   }
> > 
> >   return Hover();
> > }
> > ```
> Here's the version in which I tried to simplify this *a bit*. With the new 
> ErrorOr checks as well.
> 
> ```
> Hover clangd::getHover(ParsedAST , Position Pos, clangd::Logger ) {
>   const SourceManager  = AST.getASTContext().getSourceManager();
>   const LangOptions  = 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1071
 
+Location getDeclarationLocation(ParsedAST ,
+const SourceRange ) {

llvm::ErrorOr



Comment at: clangd/ClangdUnit.cpp:1075
+  const LangOptions  = AST.getASTContext().getLangOpts();
+  SourceLocation LocStart = ValSourceRange.getBegin();
+  SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 
0,

To fix the macro expansion crash:
```
SourceLocation LocStart = SourceMgr.getExpansionLoc(ValSourceRange.getBegin());
const FileEntry *F =
SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
```
and simple early return
```
if (!F)
  return llvm::errc::no_such_file_or_directory;
```



Comment at: clangd/ClangdUnit.cpp:1079
+  Position Begin;
+  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
+  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;

getExpansionLineNumber



Comment at: clangd/ClangdUnit.cpp:1080
+  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
+  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+  Position End;

getExpansionColumnNumber



Comment at: clangd/ClangdUnit.cpp:1082
+  Position End;
+  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
+  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;

getExpansionLineNumber



Comment at: clangd/ClangdUnit.cpp:1083
+  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
+  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Range R = {Begin, End};

getExpansionColumnNumber



Comment at: clangd/ClangdUnit.cpp:1087
+
+  const FileEntry *F =
+  SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));

You can do this earlier and simpler, see comment above.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/Protocol.cpp:498
+// Default Hover values
+Hover H;
+return json::obj{

remove, we have to return the contents of the H that was passed as parameter, 
not a new one. I hit this bug while testing with no range (hover text in 
another file)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1117
+
+  if (!DeclVector.empty()) {
+const Decl *LocationDecl = DeclVector[0];

malaperle wrote:
> I think we need to simplify this part a bit. I'll try to find a way. Feel 
> free to wait until more comments before updating.
Here's the version in which I tried to simplify this *a bit*. With the new 
ErrorOr checks as well.

```
Hover clangd::getHover(ParsedAST , Position Pos, clangd::Logger ) {
  const SourceManager  = AST.getASTContext().getSourceManager();
  const LangOptions  = AST.getASTContext().getLangOpts();
  const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
  if (!FE)
return Hover();

  SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);

  auto DeclMacrosFinder = std::make_shared(
  llvm::errs(), SourceLocationBeg, AST.getASTContext(),
  AST.getPreprocessor());
  index::IndexingOptions IndexOpts;
  IndexOpts.SystemSymbolFilter =
  index::IndexingOptions::SystemSymbolFilterKind::All;
  IndexOpts.IndexFunctionLocals = true;
  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
 DeclMacrosFinder, IndexOpts);

  auto Decls = DeclMacrosFinder->takeDecls();
  if (!Decls.empty() && Decls[0]) {
const Decl *LocationDecl = Decls[0];
std::vector HoverContents;

// Compute scope as the first "section" of the hover.
if (const NamedDecl *NamedD = dyn_cast(LocationDecl)) {
  std::string Scope = getScope(NamedD, AST.getASTContext().getLangOpts());
  if (!Scope.empty()) {
MarkedString Info = {"", "C++", "In " + Scope};
HoverContents.push_back(Info);
  }
}

// Adjust beginning of hover text depending on the presence of templates 
and comments.
TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
const Decl * BeginDecl = TDec ? TDec : LocationDecl;
SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
BeginDecl);
if (RC)
  BeginLocation = RC->getLocStart();

// Adjust end of hover text for things that have braces/bodies. We don't
// want to show the whole definition of a function, class, etc.
SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
if (auto FuncDecl = dyn_cast(LocationDecl)) {
  if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
EndLocation = FuncDecl->getBody()->getLocStart();
} else if (auto TagDeclaration = dyn_cast(LocationDecl)) {
  if (TagDeclaration->isThisDeclarationADefinition())
EndLocation = TagDeclaration->getBraceRange().getBegin();
} else if (auto NameDecl = dyn_cast(LocationDecl)) {
  SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
  LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
  if (BeforeLBraceLoc.isValid())
EndLocation = BeforeLBraceLoc;
}

SourceRange SR(BeginLocation, EndLocation);
if (SR.isValid()) {
  auto L = getDeclarationLocation(AST, SR);
  if (L) {
auto Ref = getBufferDataFromSourceRange(AST, *L);
if (Ref) {
  Hover H;
  if (SourceMgr.isInMainFile(BeginLocation))
H.range = L->range;
  MarkedString MS = {"", "C++", *Ref};
  HoverContents.push_back(MS);
  H.contents = std::move(HoverContents);
  return H;
}
  }
}
  }

  auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
  if (!MacroInfos.empty() && MacroInfos[0]) {
const MacroInfo *MacroInf = MacroInfos[0];
SourceRange SR(MacroInf->getDefinitionLoc(),
 MacroInf->getDefinitionEndLoc());
if (SR.isValid()) {
  auto L = getDeclarationLocation(AST, SR);
  if (L) {
auto Ref = getBufferDataFromSourceRange(AST, *L);
if (Ref) {
  Hover H;
  if (SourceMgr.isInMainFile(SR.getBegin()))
H.range = L->range;
  MarkedString MS = {"", "C++", "#define " + Ref->str()};
  H.contents.push_back(MS);
  return H;
}
  }
}
  }

  return Hover();
}
```



Comment at: clangd/ClangdUnit.cpp:1172
+Location L = getDeclarationLocation(AST, SR);
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);

The range could be in another file but we can only highlight things in the file 
that the user current has open. For example, if I'm foo.cpp and I hover on 
something declared in foo.h, it will change the background color in foo.cpp but 
using the line numbers of foo.h! The protocol should be more clear about this 
but since there are no Uri in the Hover struct, we can assume this range should 
only apply to the open file, i.e. the main file. So I suggest this check:
  if (SourceMgr.isInMainFile(BeginLocation))
H.range = 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 124833.
Nebiroth added a comment.
Herald added a subscriber: klimek.

Invalid FileEntries now return llvm:ErrorOr


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,56 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 611
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\n//comments test\ntemplate\nT templateTest(T foo) {\nreturn foo;}\ntemplate\nclass classTemplateTest {\npublic: T test;};\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;\nint temp = 5;\ntemplateTest(temp);classTemplateTest test;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":20},"start":{"character":0,"line":20
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":22,"character":15}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":23,"character":10}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":24,"character":13}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":25,"character":1}}}
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":26,"character":8}}}
+# 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1029
+  SourceLocation LocStart = ValSourceRange.getBegin();
+  SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 
0,
+ SourceMgr, LangOpts);

  const FileEntry *F =
  SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
  if (!F)
return llvm::errc::no_such_file_or_directory;



Comment at: clangd/ClangdUnit.cpp:1039
+  Location L;
+  if (const FileEntry *F =
+  SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart))) {

FE can be null. How about returning a llvm::ErrorOr ?



Comment at: clangd/ClangdUnit.cpp:1242
+
+  const FileEntry *FE =
+  AST.getASTContext().getSourceManager().getFileManager().getFile(

FE can be null. How about returning a llvm::ErrorOr ?


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.h:320
+
+StringRef getDataBufferFromSourceRange(ParsedAST , SourceRange SR,
+   Location L);

I think this can be only in the source file, in a an anonymous namespace.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1135
+RawComment *RC =
+AST.getASTContext().getRawCommentForDeclNoCache(LocationDecl);
+if (RC) {

Not sure why but I don't get the comments when I hover on "push_back" but I do 
get it on many other methods. This can be investigated separately I think. It 
could be a bug in getRawCommentForDeclNoCache


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Protocol.h:453
+struct MarkedString {
+  /**
+   * MarkedString can be used to render human readable text. It is either a

The doc should use /// like the others


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1173
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);
+  }

malaperle wrote:
> malaperle wrote:
> > I get the same crash as I mentioned before if I hover on the class in 
> > "isa(D)", from Clangdunit.cpp (disclaimer: I'm testing with 
> > older source so it might not be there anymore.)
> > 
> > I have still yet to investigate this.
> I think it has to do with macro expansions, the plot thickens..
ObjCMethodDecl is forward-declared in astfwd.h as part of a macro expansion. We 
have to either use the expansion loc or spelling loc to get the appropriate 
file entry. Probably expansion loc makes more sense but I'll test a bit.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:1173
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);
+  }

malaperle wrote:
> I get the same crash as I mentioned before if I hover on the class in 
> "isa(D)", from Clangdunit.cpp (disclaimer: I'm testing with 
> older source so it might not be there anymore.)
> 
> I have still yet to investigate this.
I think it has to do with macro expansions, the plot thickens..


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdLSPServer.cpp:245
+
+  C.reply(Hover::unparse(H->Value));
+}

we need to check the "Expected" here, so

  if (!H) {
C.replyError(ErrorCode::InternalError,
 llvm::toString(H.takeError()));
return;
  }

Unless you can think of other error situations?



Comment at: clangd/ClangdServer.cpp:448
   });
+
   return make_tagged(std::move(Result), TaggedFS.Tag);

revert



Comment at: clangd/ClangdServer.cpp:450
 }
-
 llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {

revert



Comment at: clangd/ClangdUnit.cpp:943
 
-/// Finds declarations locations that a given source location refers to.
-class DeclarationLocationsFinder : public index::IndexDataConsumer {
-  std::vector DeclarationLocations;
+/// Finds declarations  and macros that a given source location refers to.
+class DeclarationAndMacrosFinder : public index::IndexDataConsumer {

extra space before the "and"



Comment at: clangd/ClangdUnit.cpp:1092
+  // Default Hover values
+  std::vector EmptyVector;
+  Hover H;

not used, remove



Comment at: clangd/ClangdUnit.cpp:1094
+  Hover H;
+  StringRef Ref;
+

Can this be named something more descriptive? HoverText?



Comment at: clangd/ClangdUnit.cpp:1117
+
+  if (!DeclVector.empty()) {
+const Decl *LocationDecl = DeclVector[0];

I think we need to simplify this part a bit. I'll try to find a way. Feel free 
to wait until more comments before updating.



Comment at: clangd/ClangdUnit.cpp:1173
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);
+  }

I get the same crash as I mentioned before if I hover on the class in 
"isa(D)", from Clangdunit.cpp (disclaimer: I'm testing with 
older source so it might not be there anymore.)

I have still yet to investigate this.



Comment at: clangd/ClangdUnit.cpp:1238
+/// Returns the file buffer data that the given SourceRange points to.
+StringRef clangd::getDataBufferFromSourceRange(ParsedAST , SourceRange SR,
+   Location L) {

getBufferDataFromSourceRange, to be consistent with 
getSourceManager().getBufferData



Comment at: clangd/ClangdUnit.cpp:1239
+StringRef clangd::getDataBufferFromSourceRange(ParsedAST , SourceRange SR,
+   Location L) {
+  StringRef Ref;

I think the SourceRange SR should be used here, not Location.



Comment at: clangd/ClangdUnit.h:318
+
+std::string getScope(const NamedDecl *ND, const LangOptions );
+

I think this can be only in the source file, in a an anonymous namespace.



Comment at: clangd/Protocol.cpp:1029
+{"contents", json::ary(H.contents)},
+{"range", DefaultRange},
+};

I don't think "range" should be outputted here



Comment at: test/clangd/hover.test:10
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define
 MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid 
anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\nint 
main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* 
Params;\nParams->anotherOperation();\nMACRO;}\n"}}}
+

probably the test should include templates and comments?



Comment at: test/clangd/hover.test:15
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define
 MACRO 
1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0

copy/pasted comment?



Comment at: test/clangd/hover.test:21
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":14,"character":1}}}
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int 
a"}],"range":{"end":{"character":5,"line":13},"start":{"character":0,"line":13

copy/pasted comment?



Comment at: test/clangd/hover.test:27
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":15,"character":15}}}
+# Go to local variable
+# CHECK: 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:230
+  std::vector LocationVector;
+  auto test = Items->Value;
+

remove



Comment at: clangd/ClangdLSPServer.cpp:236
+
+  C.reply(json::ary(LocationVector));
 }

I think you can just do Items->Value here and not have LocationVector at all.



Comment at: clangd/ClangdLSPServer.cpp:251
+  Position{Params.position.line, Params.position.character});
+  if (!H)
+return C.replyError(ErrorCode::InvalidParams,

I think we should return an empty Hover here and not an error. Since the empty 
Hover is already generated, this would mean simply removing those two lines.



Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"

Remove include?



Comment at: clangd/ClangdServer.cpp:432
 ClangdServer::findDefinitions(PathRef File, Position Pos) {
+
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);

revert



Comment at: clangd/ClangdServer.cpp:447
   });
+
   return make_tagged(std::move(Result), TaggedFS.Tag);

revert



Comment at: clangd/ClangdServer.cpp:517
+  Range DefaultRange = {BeginPosition, EndPosition};
+  Hover FinalHover = {EmptyVector, DefaultRange};
+

I think you remove all default values and just do 
Hover FinalHover;



Comment at: clangd/ClangdServer.cpp:527
+  return;
+// Only show hover for the first found definition, we could potentially
+// expand this in a later patch.

I don't think this comment belong here anymore?



Comment at: clangd/ClangdUnit.cpp:1187
+  } else if (auto ClassDecl = dyn_cast(LocationDecl)) {
+if (!ClassDecl->isInterface() && !ClassDecl->isUnion() &&
+ClassDecl->isThisDeclarationADefinition())

what's wrong with interface and union?



Comment at: clangd/ClangdUnit.cpp:1246
+  Contexts.push_back(Ctx);
+  Ctx = Ctx->getParent();
+}

Ctx is never used so you can remove lines 1231 to 1247



Comment at: clangd/ClangdUnit.cpp:942
 
 /// Finds declarations locations that a given source location refers to.
 class DeclarationLocationsFinder : public index::IndexDataConsumer {

This comment isn't accurate anymore.



Comment at: clangd/ClangdUnit.cpp:943
 /// Finds declarations locations that a given source location refers to.
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::vector DeclarationDecls;

I think this should be renamed, it doesn't find locations for declarations 
anymore. I don't know what to call it, hmmm DeclOrMacroAtLocationFinder? :)



Comment at: clangd/ClangdUnit.cpp:944
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
-  std::vector DeclarationLocations;
+  std::vector DeclarationDecls;
+  std::vector DeclarationMacroInfs;

just Decls?



Comment at: clangd/ClangdUnit.cpp:945
+  std::vector DeclarationDecls;
+  std::vector DeclarationMacroInfs;
   const SourceLocation 

just MacroInfos?



Comment at: clangd/ClangdUnit.cpp:986
 
+  std::vector getDeclarationDecls() { return DeclarationDecls; };
+  std::vector getMacroInfos() {

I don't think that getter is necessary, only use the take



Comment at: clangd/ClangdUnit.cpp:987
+  std::vector getDeclarationDecls() { return DeclarationDecls; };
+  std::vector getMacroInfos() {
+return DeclarationMacroInfs;

I don't think that getter is necessary, only use the take



Comment at: clangd/ClangdUnit.cpp:1044
+  Location L;
+  if (const FileEntry *F =
+  SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart))) {

It looks like this sometimes return null and we don't handle this well. Need to 
investigate this more though. See later comment.



Comment at: clangd/ClangdUnit.cpp:1086
+  for (auto Item : MacroInfos) {
+SourceRange SR =
+SourceRange(Item->getDefinitionLoc(), Item->getDefinitionEndLoc());

You can omit the "= SourceRange"



Comment at: clangd/ClangdUnit.cpp:1102
+  Range DefaultRange = {BeginPosition, EndPosition};
+  Hover H = {EmptyVector, DefaultRange};
+  unsigned Start, End;

you can remove the lines before, just
"Hover H;" is OK
Range is optional and the vector is already initialized to empty.



Comment at: clangd/ClangdUnit.cpp:1103
+  Hover H = {EmptyVector, DefaultRange};
+  unsigned Start, End;
+  StringRef Ref;

I think those should be declared where they are 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 123958.
Nebiroth marked 10 inline comments as done.
Nebiroth added a comment.

Removed accidentally included files
Fixed some coding standard issues
Removed getDeclarationLocation declaration from header file
Replaced getFunctionComments with clang implementation
onCodeHover now follows error-reporting pattern


https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -5,6 +5,7 @@
 Content-Length: 143
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
+
 #  CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
@@ -30,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
@@ -40,6 +41,7 @@
 # CHECK-NEXT:  "textDocumentSync": 1
 # CHECK-NEXT:}
 # CHECK-NEXT:  }
+
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,52 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 407
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":14,"character":1}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":13},"start":{"character":0,"line":13
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":15,"character":15}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":16,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":13}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() 

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

I haven't looked into the `getHover` method's body yet, but here are some 
comments about other parts.
Good work, BTW, this looks close to being ready.




Comment at: clangd/ClangdServer.cpp:511
+Tagged ClangdServer::findHover(PathRef File, Position Pos) {
+
+  //Default Hover values

NIT: could you remove the empty newlines at the start of the methods?



Comment at: clangd/ClangdServer.cpp:514
+  std::vector EmptyVector;
+  Position BeginPosition = {0,0};
+  Position EndPosition = {0,0};

Let's follow error-reporting pattern used in other `ClangdServer` features.
We should return `llvm::Expected` and do `Ctx.replyError` on error return 
values.



Comment at: clangd/ClangdUnit.cpp:959
+std::sort(DeclarationDecls.begin(), DeclarationDecls.end());
+auto last = std::unique(DeclarationDecls.begin(), DeclarationDecls.end());
+DeclarationDecls.erase(last, DeclarationDecls.end());

NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but 
probably a good place to fix the naming anyway).



Comment at: clangd/ClangdUnit.cpp:967
+// This can happen when nodes in the AST are visited twice.
+std::sort(DeclarationMacroInfs.begin(), DeclarationMacroInfs.end());
 auto last =

NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but 
probably a good place to fix the naming anyway).



Comment at: clangd/ClangdUnit.cpp:1100
+
+  const SourceManager  = AST.getASTContext().getSourceManager();
+  const LangOptions  = AST.getASTContext().getLangOpts();

I think this functionality is already implemented in clang. Could you use 
`ASTContext::getRawCommentForAnyRedecl`?



Comment at: clangd/ClangdUnit.cpp:1199
+  } else if (dyn_cast(LocationDecl)) {
+// TODO: find a way to get the hover for the type that is being aliased
+SR = LocationDecl->getSourceRange();

NIT: we use `FIXME`



Comment at: clangd/ClangdUnit.h:323
+
+Location getDeclarationLocation(ParsedAST , const SourceRange 
);
+

Could we put `getDeclarationLocation` and `getFunctionComments` in the cpp file 
instead? They're just implementation detail of `findDefinitions`/`getHover`, 
right?



Comment at: clangd/MyClass.cpp:1
+#include "MyClass.h"
+

Remove it.



Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"

We don't need `SourceLocation`  here



Comment at: clangd/Protocol.h:27
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"

This include is redundant


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added a comment.

Forgot to mention this last patch also added support for displaying function 
comments above the function definition displayed.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 123662.
Nebiroth marked 10 inline comments as done.
Nebiroth added a comment.

Removed all std::pair objects
Fixed and updated all tests
findHover doesn't call findDefinitions anymore
DeclarationLocationsFinder fills two Decl and MacroInfos vectors instead of 
giving out Location objects
Removed constructors for structs MarkedString and Hover
Moved code modifying SourceRange for hover into getHover()


https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/MyClass.cpp
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/impl.cpp
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -5,6 +5,7 @@
 Content-Length: 143
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
+
 #  CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
@@ -30,6 +31,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -30,6 +30,7 @@
 # CHECK-NEXT:  "clangd.applyFix"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:	   "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
@@ -40,6 +41,7 @@
 # CHECK-NEXT:  "textDocumentSync": 1
 # CHECK-NEXT:}
 # CHECK-NEXT:  }
+
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,52 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 407
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":14,"character":1}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":13},"start":{"character":0,"line":13
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":15,"character":15}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":16,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":13}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 6 inline comments as done.
Nebiroth added inline comments.



Comment at: clangd/ClangdUnit.cpp:981
+}
+
 if (isSearchedLocation(FID, Offset)) {

malaperle wrote:
> I think we need to do a check that the computed SourceRange is valid 
> (isValid) in case we get it wrong. Otherwise, it might not go well later when 
> we use the SourceLocation to convert to lines, etc.
Do we just output a default Range values when SR isn't valid?


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 25 inline comments as done.
Nebiroth added inline comments.



Comment at: clangd/ClangdServer.cpp:360
+
+  auto FileContents = DraftMgr.getDraft(File);
+  assert(FileContents.Draft &&

malaperle wrote:
> Why are you adding this? Is this coming from another commit?
> It looks like it makes tests fail:
> <-- 
> {"jsonrpc":"2.0","id":2,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///doesnotexist.cpp"},"position":{"line":4,"character":7}}}
> clangd: ../tools/clang/tools/extra/clangd/ClangdServer.cpp:362: 
> llvm::Expected  const clang::Decl*> > > > 
> clang::clangd::ClangdServer::findDefinitions(clang::clangd::PathRef, 
> clang::clangd::Position): Assertion `FileContents.Draft && "findDefinitions 
> is called for non-added document"' failed.
> /home/emalape/git/llvm/tools/clang/tools/extra/test/clangd/definitions.test:169:10:
>  error: expected string not found in input
> 
Accidental addition.



Comment at: clangd/ClangdServer.h:281
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
-  llvm::Expected> findDefinitions(PathRef File,
+  llvm::Expected malaperle wrote:
> > Location can be deduced from Decl*. I don't think this should be a pair. I 
> > also think we need a more general finder that just gets either the Decl* or 
> > MacroDef* at the "cursor". I think CXCursor does something similar in that 
> > it stores either Decl* or MacroDef*. I'm not sure it would be worth moving 
> > CXCursor from libclang but perhaps something stripped down for Clangd would 
> > be OK. This new finder will be able to be reused by findDefinition, 
> > highlights, hover, references and more.
> > We can make one patch that refactors the existing code so that 
> > findDefinitions uses this new finder class.
> We should not return `Decl*` from `ClangdServer::findDefinitions`, it's an 
> internal clang object (there are many other reasons why it's bad). 
> 
> It's ok the change the `findDefinitions` helper function inside 
> `ClangdUnit.cpp`, though. Please change it instead.
> 
> As for the `CXCursor`, I think the simplest approach would be to return two 
> vectors (`vector` and `vector`). I think clangd would 
> eventually get its own `CXCursor`-like things, possible reusing code or 
> wrapping CXCursor. But for now let's keep things simple.
We would have to handle MacroDef occurrences for this to work first. In other 
words, we would have to implement handleMacroDefOccurence in a similar way to 
handleDeclOccurence in DeclarationLocationsFinder. The document highlights 
feature suffers from the exact same problem. I'm planning on working on this 
feature in the near future.



Comment at: clangd/ClangdUnit.cpp:941
+
+// Keep default value.
+SourceRange SR = D->getSourceRange();

malaperle wrote:
> This code doesn't belong here. The hover feature and "find definition" do not 
> need the same range. The hover basically wants everything up to the first 
> left brace but "find definitions" wants everything up to the closing brace. 
> So having this code here will make "find definition" less correct.
Agreed, I'll revert the code to normal here and move this patch's code to 
getHover().


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for late response, was on vacation.
+1 to all @malaperle 's comments.

Here are some extra quick comments, will take a closer look later.




Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"

malaperle wrote:
> I don't know if the intention was to keep the protocol completely out of the 
> ClangdServer class. Maybe someone else can clarify. But either way, I see 
> that ClangdUnit does include "Protocol.h" so it's probably OK for now.
We don't want dependencies on JSON parsing/unparsing and the protocol itself. 
We reuse some of the LSP class definitions to avoid code duplication, though.

This include is redundant, though, as `ClangdServer.h` already include 
`Protocol.h`. 



Comment at: clangd/ClangdServer.h:281
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
-  llvm::Expected> findDefinitions(PathRef File,
+  llvm::Expected Location can be deduced from Decl*. I don't think this should be a pair. I 
> also think we need a more general finder that just gets either the Decl* or 
> MacroDef* at the "cursor". I think CXCursor does something similar in that it 
> stores either Decl* or MacroDef*. I'm not sure it would be worth moving 
> CXCursor from libclang but perhaps something stripped down for Clangd would 
> be OK. This new finder will be able to be reused by findDefinition, 
> highlights, hover, references and more.
> We can make one patch that refactors the existing code so that 
> findDefinitions uses this new finder class.
We should not return `Decl*` from `ClangdServer::findDefinitions`, it's an 
internal clang object (there are many other reasons why it's bad). 

It's ok the change the `findDefinitions` helper function inside 
`ClangdUnit.cpp`, though. Please change it instead.

As for the `CXCursor`, I think the simplest approach would be to return two 
vectors (`vector` and `vector`). I think clangd would 
eventually get its own `CXCursor`-like things, possible reusing code or 
wrapping CXCursor. But for now let's keep things simple.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdLSPServer.cpp:205
+
+  if (!(H.contents[0].codeBlockLanguage == "" &&
+H.contents[0].markdownString == "" &&

I think we should just always call unparse and handle the various cases in 
there, because we always have to return a Hover anyway (see comment below).



Comment at: clangd/ClangdLSPServer.cpp:210
+  else
+C.reply("[]");
+}

This is an invalid response because we still have to return a Hover, it is not 
optional. We can return a empty "contents" array though, so I think it would 
look like: { contents: [] }



Comment at: clangd/ClangdServer.cpp:360
+
+  auto FileContents = DraftMgr.getDraft(File);
+  assert(FileContents.Draft &&

Why are you adding this? Is this coming from another commit?
It looks like it makes tests fail:
<-- 
{"jsonrpc":"2.0","id":2,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///doesnotexist.cpp"},"position":{"line":4,"character":7}}}
clangd: ../tools/clang/tools/extra/clangd/ClangdServer.cpp:362: 
llvm::Expected > > > 
clang::clangd::ClangdServer::findDefinitions(clang::clangd::PathRef, 
clang::clangd::Position): Assertion `FileContents.Draft && "findDefinitions is 
called for non-added document"' failed.
/home/emalape/git/llvm/tools/clang/tools/extra/test/clangd/definitions.test:169:10:
 error: expected string not found in input




Comment at: clangd/ClangdServer.cpp:445
+
+  std::vector Contents;
+  MarkedString MS = MarkedString("", "");

I would just remove all this initialization code and leave only Hover 
FinalHover;
The default constructor of Hover should be equivalent to "empty" result (but 
still valid). More below.



Comment at: clangd/ClangdServer.cpp:446
+  std::vector Contents;
+  MarkedString MS = MarkedString("", "");
+  Contents.push_back(MS);

This is the same as the default contructor, so just 
MarkedString MS;
would have been OK.



Comment at: clangd/ClangdServer.cpp:449
+  Range R;
+  // Hover FinalHover(MS, R);
+  Hover FinalHover(Contents, R);

remove commented line



Comment at: clangd/ClangdServer.cpp:450
+  // Hover FinalHover(MS, R);
+  Hover FinalHover(Contents, R);
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);

I actually think you should leave it with the default contructor, so that 
"contents" is an empty array, which is what should be returned when there is no 
hover.



Comment at: clangd/ClangdServer.cpp:454
+  std::shared_ptr Resources = Units.getFile(File);
+  assert(Resources && "Calling findDefinitions on non-added file");
+

findDefinitions -> findHover



Comment at: clangd/ClangdServer.cpp:456
+
+  std::vector> Result;
+

You can move this inside the lambda.



Comment at: clangd/ClangdServer.cpp:459
+  Resources->getAST().get()->runUnderLock(
+  [Pos, , , this](ParsedAST *AST) {
+if (!AST)

Don't need to capture Result if it's declared inside the lambda (previous 
comment)



Comment at: clangd/ClangdServer.cpp:464
+if (!Result.empty()) {
+  FinalHover = clangd::getHover(*AST, Result[0]);
+}

Maybe we should put a comment here that we only show the hover for the first 
"definition" here and that perhaps more could be shown in the future.



Comment at: clangd/ClangdServer.h:67
+
+  // template 
 

remove



Comment at: clangd/ClangdServer.h:281
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
-  llvm::Expected> findDefinitions(PathRef File,
+  llvm::Expected

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added a comment.

I also have a quick patch that supports displaying comments preceding the 
declaration of a function. Once the review comments have been addressed for 
this revision I will submit it.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 120894.
Nebiroth added a comment.

Code hover now displays declaration of symbol instead of source code by default.
Code hover now displays context information such as namespace and class name.
Array of MarkedString objects is now sent as response in JSON.


https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -5,16 +5,17 @@
 Content-Length: 143
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 535
+# CHECK: Content-Length: 568
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
 # CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
-# CHECK:   "definitionProvider": true
+# CHECK:   "definitionProvider": true,
+# CHECK:   "hoverProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -5,16 +5,17 @@
 Content-Length: 142
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":"","rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 535
+# CHECK: Content-Length: 568
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
 # CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
-# CHECK:   "definitionProvider": true
+# CHECK:   "definitionProvider": true,
+# CHECK:   "hoverProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,52 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 407
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;}\n"}}}
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": ["#define statement",{"language": "C++", "value": "MACRO 1"}], "range": {"start": {"line": 0, "character": 8}, "end": {"line": 0, "character": 15
+
+Content-Length: 144
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":14,"character":1}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": [{"language": "C++", "value": "int a"}], "range": {"start": {"line": 13, "character": 0}, "end": {"line": 13, "character": 5
+
+Content-Length: 145
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":15,"character":15}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": ["In namespace named ns1",{"language": "C++", "value": "int test = 5"}], "range": {"start": {"line": 2, "character": 0}, "end": {"line": 2, "character": 12
+
+Content-Length: 145
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#903607, @malaperle wrote:

> Exactly! Although the without-language part, maybe it'll be "scope" 
> information first. Maybe documentation/comments in a second patch.


Sure, whatever works/looks best.

In https://reviews.llvm.org/D35894#903611, @Nebiroth wrote:

> I'm sitting on a code hover patch that does more or less what you just 
> described. One MarkedString with it's language field set to "C++" returns the 
> appropriate code snippet for the code hover and another MarkedString with 
> it's language field set to "markdown" returns the associated scope.


Sounds cool, looking forward to reviewing it.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added a comment.

In https://reviews.llvm.org/D35894#903594, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
>
> > I think he meant to have multiple sections in the hover, one C/C++ and one 
> > not. But we noticed you can have an array of MarkedString in Hover so it 
> > should be fine.
>
>
> I totally misunderstood the question. Good to know it all works without 
> changing the LSP.
>  Just to make sure I got it right this time: the use-case is to, e.g., return 
> both a code snippet (having `"language": cpp`) and a documentation string 
> (without `language`) as a result of hover request?


That's about it.

I'm sitting on a code hover patch that does more or less what you just 
described. One MarkedString with it's language field set to "C++" returns the 
appropriate code snippet for the code hover and another MarkedString with it's 
language field set to "markdown" returns the associated scope.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#903594, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
>
> > I think he meant to have multiple sections in the hover, one C/C++ and one 
> > not. But we noticed you can have an array of MarkedString in Hover so it 
> > should be fine.
>
>
> I totally misunderstood the question. Good to know it all works without 
> changing the LSP.
>  Just to make sure I got it right this time: the use-case is to, e.g., return 
> both a code snippet (having `"language": cpp`) and a documentation string 
> (without `language`) as a result of hover request?


Exactly! Although the without-language part, maybe it'll be "scope" information 
first. Maybe documentation/comments in a second patch.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#903552, @malaperle wrote:

> I think he meant to have multiple sections in the hover, one C/C++ and one 
> not. But we noticed you can have an array of MarkedString in Hover so it 
> should be fine.


I totally misunderstood the question. Good to know it all works without 
changing the LSP.
Just to make sure I got it right this time: the use-case is to, e.g., return 
both a code snippet (having `"language": cpp`) and a documentation string 
(without `language`) as a result of hover request?


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#903542, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#895023, @Nebiroth wrote:
>
> > I would like some feedback/suggestions on whether it's possible to "append" 
> > information to a MarkedString to be displayed on client?
>
>
> You mean append **after** returning `MarkedString` to the client? I don't 
> think that's possible.
>  Under what circumstances this could be useful? In general, it seems 
> perfectly fine if we send the whole description+documentation right away.


I think he meant to have multiple sections in the hover, one C/C++ and one not. 
But we noticed you can have an array of MarkedString in Hover so it should be 
fine.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#895023, @Nebiroth wrote:

> I would like some feedback/suggestions on whether it's possible to "append" 
> information to a MarkedString to be displayed on client?


You mean append **after** returning `MarkedString` to the client? I don't think 
that's possible.
Under what circumstances this could be useful? In general, it seems perfectly 
fine if we send the whole description+documentation right away.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added a comment.

Bumping this.

I've worked on a patch for this feature that currently supports showing the 
declaration of whatever is being hovered on instead of the raw source code. It 
also has basic support to distinguish declarations in types ( class/struct, 
namespace, global variable,  typedef, enum, namespace, etc.) so that we can 
have a 'kind' of a symbol like mentioned in the comments above.

The step I am at right now is figuring out a way to display this information 
inside the box that will be displayed to the client once a response 
(MarkedString) is sent back by the server. I would like some 
feedback/suggestions on whether it's possible to "append" information to a 
MarkedString to be displayed on client? I was also considering extending LSP to 
include more than just a MarkedString for the response in order to have 
something a bit more flexible to use in-client.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#830178, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#829190, @malaperle wrote:
>
> > @Nebiroth I think it's OK to put this on hold until we make the "semantic" 
> > hover and figure out how to have both. From our perspective, this is going 
> > beyond parity of what we had before but it's seems like the right thing to 
> > do.
>
>
> BTW, just noticed that VS Code shows the range reported by `findDefinitions` 
> when you hover over a symbol and holding Ctrl.
>  Would calling `findDefinitions` help you to do the same thing? That would 
> allow to get closer to feature parity with CDT without implementing semantic 
> hover in clangd.


That looks very interesting, I had no idea. We'll have to try that. Thanks!


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D35894#829190, @malaperle wrote:

> @Nebiroth I think it's OK to put this on hold until we make the "semantic" 
> hover and figure out how to have both. From our perspective, this is going 
> beyond parity of what we had before but it's seems like the right thing to do.


BTW, just noticed that VS Code shows the range reported by `findDefinitions` 
when you hover over a symbol and holding Ctrl.
Would calling `findDefinitions` help you to do the same thing? That would allow 
to get closer to feature parity with CDT without implementing semantic hover in 
clangd.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

@Nebiroth I think it's OK to put this on hold until we make the "semantic" 
hover and figure out how to have both. From our perspective, this is going 
beyond parity of what we had before but it's seems like the right thing to do.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#829109, @ilya-biryukov wrote:

> > I think all of those would be great. Our objective is to bring basic but 
> > correct features that will put us close to parity with Eclipse CDT, so that 
> > our users can transition. In CDT only the "raw" source is shown, similar to 
> > this patch (except in CDT it attaches comment nodes so you get the few 
> > lines of comments before). I think priority wise, we would likely want to 
> > tackle other LSP features first before adding more things to the hover, 
> > (References, Open Definition, etc).
>
> Don't get me wrong, I'm not trying to get all of what I described in the 
> first CL, but I definitely think that's the experience we want from hovers 
> and we should lay out foundation to make it happen.
>  I.e. here is a meta-description of algorithm for hover that is easy to 
> implement and extend later:
>
> 1. Resolve reference under cursor. Result: `Decl` or `MacroDefinition` that 
> the symbol under cursor refers to (possibly a list of results).
>   - This part should be reused between `findDefinitions` and hovers. It's 
> already there, we just need to extract an interface that provides `Decl` and 
> `MacroDefinition` instead of their `Range`.
> 2. Transform result of step 1 (`Decl` or `MacroDefinition`) into a hover 
> description (i.e. into a MarkedString).
>   - This part can be super-simple in the first commit, i.e. it may only 
> output a source code of declaration and a symbol kind (i.e. local variable, 
> parameter, class, macro, etc). That could be easily extended in the future.
>
> > I do find that just showing the raw code is lacking in many situations for 
> > example I have found myself having to figure out which namespace a symbol 
> > was in by hand so it's very much desirable to add more to the hover.
>
> Sure, even though showing source code is still fine most of the time.
>  I also wanted to stress that we shouldn't too much. Let's not show the body 
> of the function we're referring to as it may be very large and it's 
> irrelevant most of the time. If a user really wants to see definition, he can 
> jump to it using `findDefinitions`.


As a user I want to be able to peek at the code without changing context, not 
all the time though for sure. Eclipse actually has two hovers: a "semantic" one 
which in Java includes the package, type, visibility, javadoc, etc, this is 
what you describe and the default in Java (and it should be for C++ if it 
existed!). And there is also a source hover which just outputs the source code 
of that symbols, which you enable by holding shift (the only one implemented in 
the case of C++). The objective is to have both in Clangd but the latter is 
much more low hanging. Whether of not the body of a function is large doesn't 
matter because clients either crop it or add scollbars to the hover (Eclipse, 
VS Code, Theia).


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> I think all of those would be great. Our objective is to bring basic but 
> correct features that will put us close to parity with Eclipse CDT, so that 
> our users can transition. In CDT only the "raw" source is shown, similar to 
> this patch (except in CDT it attaches comment nodes so you get the few lines 
> of comments before). I think priority wise, we would likely want to tackle 
> other LSP features first before adding more things to the hover, (References, 
> Open Definition, etc).

Don't get me wrong, I'm not trying to get all of what I described in the first 
CL, but I definitely think that's the experience we want from hovers and we 
should lay out foundation to make it happen.
I.e. here is a meta-description of algorithm for hover that is easy to 
implement and extend later:

1. Resolve reference under cursor. Result: `Decl` or `MacroDefinition` that the 
symbol under cursor refers to (possibly a list of results).
  - This part should be reused between `findDefinitions` and hovers. It's 
already there, we just need to extract an interface that provides `Decl` and 
`MacroDefinition` instead of their `Range`.
2. Transform result of step 1 (`Decl` or `MacroDefinition`) into a hover 
description (i.e. into a MarkedString).
  - This part can be super-simple in the first commit, i.e. it may only output 
a source code of declaration and a symbol kind (i.e. local variable, parameter, 
class, macro, etc). That could be easily extended in the future.

> I do find that just showing the raw code is lacking in many situations for 
> example I have found myself having to figure out which namespace a symbol was 
> in by hand so it's very much desirable to add more to the hover.

Sure, even though showing source code is still fine most of the time.
I also wanted to stress that we shouldn't too much. Let's not show the body of 
the function we're referring to as it may be very large and it's irrelevant 
most of the time. If a user really wants to see definition, he can jump to it 
using `findDefinitions`.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#828702, @ilya-biryukov wrote:

> I just wanted to take a step back and discuss what we want to get from code 
> hover in the first place.
>
> I would expect a typical code hover to be a bit more involved and include:
>
> - "kind" of a symbol under hover (i.e. a MACRO, global function, typedef, 
> class/struct),
> - type of the symbol, if applicable (i.e. types for local vars, typedefs, 
> etc.),
> - name of the symbol,
> - containing namespace/class information (i.e. "member of namespace std", 
> "member of class std::vector"),
> - doc comment, if any.
>
>   We could start with a subset of those.


I think all of those would be great. Our objective is to bring basic but 
correct features that will put us close to parity with Eclipse CDT, so that our 
users can transition. In CDT only the "raw" source is shown, similar to this 
patch (except in CDT it attaches comment nodes so you get the few lines of 
comments before). I think priority wise, we would likely want to tackle other 
LSP features first before adding more things to the hover, (References, Open 
Definition, etc).
I do find that just showing the raw code is lacking in many situations for 
example I have found myself having to figure out which namespace a symbol was 
in by hand so it's very much desirable to add more to the hover.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a reviewer: ilya-biryukov.
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.

I just wanted to take a step back and discuss what we want to get from code 
hover in the first place.

I would expect a typical code hover to be a bit more involved and include:

- "kind" of a symbol under hover (i.e. a MACRO, global function, typedef, 
class/struct),
- type of the symbol, if applicable (i.e. types for local vars, typedefs, etc.),
- name of the symbol,
- containing namespace/class information (i.e. "member of namespace std", 
"member of class std::vector"),
- doc comment, if any.

We could start with a subset of those.

This could be implemented by reusing the code of `findDefinitions` that gets 
all `Decl` and `MacroDefinition` instances, referenced in the current location.
When you have a `Decl` or `MacroDefinition`, you could pretty-print the hover 
text.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.

Can you also update your code with the latest SVN trunk? The patch does not 
apply cleanly anymore.




Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"

I don't know if the intention was to keep the protocol completely out of the 
ClangdServer class. Maybe someone else can clarify. But either way, I see that 
ClangdUnit does include "Protocol.h" so it's probably OK for now.



Comment at: clangd/ClangdServer.cpp:297
+  
+  MarkedString MS = MarkedString("", "");
+  Range R;

Use default constructor for MarkedString instead? MarkedString MS;



Comment at: clangd/ClangdUnit.h:72
 
+  Hover getHover(Location L){
+

Move definition to ClangdUnit.cpp ?



Comment at: clangd/ClangdUnit.h:74
+
+MarkedString MS = MarkedString("", "");
+Range R;

MarkedString MS;



Comment at: clangd/ClangdUnit.h:76
+Range R;
+const FileEntry *FE = Unit->getFileManager().getFile(L.uri.file);
+FileID id = Unit->getSourceManager().translateFile(FE);

I don't know if you have access to a FileManager with the latest SVN trunk.



Comment at: clangd/ClangdUnit.h:82
+ref = ref.slice(start, end);
+MS = MarkedString("C++", ref);
+R = L.range;

MS = {"", "C++", ref};?



Comment at: clangd/ClangdUnit.h:87
+  }
+  unsigned start;
+  unsigned end;

I don't think those two are used (start,end). But if they are, those fields 
should not be in ClangdUnit.h



Comment at: clangd/Protocol.h:26
 #include "llvm/Support/YAMLParser.h"
+#include "clang/Basic/SourceLocation.h"
 #include 

not needed?



Comment at: clangd/Protocol.h:309
+struct MarkedString {
+  /**
+ * MarkedString can be used to render human readable text. It is either a

I don't think we should copy big parts of the documentation verbatim, 
otherwise, we'd have to distribute part of Clangd under the license for the 
LSP. I'm not familiar with "Creative Commons Attribution 3.0", etc to really 
know if you can copy everything and change the license to the LLVM's but I 
think it's safe to say that we can't.



Comment at: clangd/Protocol.h:329
+  MarkedString(std::string markdown)
+  : markdownString(markdown), codeBlockLanguage(""), codeBlockValue("") {}
+

I don't think the constructors bring much, I think they should be removed.



Comment at: clangd/Protocol.h:331
+
+  MarkedString(std::string blockLanguage, std::string blockValue)
+  : markdownString(""), codeBlockLanguage(blockLanguage),

remove?



Comment at: clangd/Protocol.h:344
+
+  Hover(MarkedString ms, Range r) : contents(ms), range(r) {}
+

MS, R  (first letters of params/local vars is capitalized)



Comment at: clangd/clients/clangd-vscode/src/extension.ts:3
 import * as vscodelc from 'vscode-languageclient';
+import * as vscodejsonrpc from 'vscode-jsonrpc';
 

remove?


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 108997.
Nebiroth added a comment.

Implemention of Code Hover as described in LSP definition.
Removed unintentional changes to formatting.
Removed file id field in Location struct.


https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/clients/clangd-vscode/src/extension.ts
  test/clangd/hover.test

Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,26 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int main() {\nint a;\na;\n}"}, "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int a"}, "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5
+
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -1,5 +1,6 @@
 import * as vscode from 'vscode';
 import * as vscodelc from 'vscode-languageclient';
+import * as vscodejsonrpc from 'vscode-jsonrpc';
 
 /**
  * Method to get workspace configuration option
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
 JSONOutput ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
 JSONOutput ) = 0;
+  virtual void onCodeHover(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput ) = 0;
 };
 
 void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,24 @@
   ProtocolCallbacks 
 };
 
+struct CodeHoverHandler : Handler {
+  CodeHoverHandler(JSONOutput , ProtocolCallbacks )
+  : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentPositionParams::parse(Params);
+if (!TDPP) {
+  Output.log("Failed to decode TextDocumentPositionParams!\n");
+  return;
+}
+
+Callbacks.onCodeHover(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks 
+};
+
 } // namespace
 
 void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +257,7 @@
   llvm::make_unique(Out, Callbacks));
   Dispatcher.registerHandler("textDocument/definition",
   llvm::make_unique(Out, Callbacks));
+  Dispatcher.registerHandler(
+  "textDocument/hover",
+  llvm::make_unique(Out, Callbacks));
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -23,6 +23,7 @@
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"
+#include "clang/Basic/SourceLocation.h"
 #include 
 #include 
 
@@ -304,7 +305,59 @@
   parse(llvm::yaml::MappingNode *Params);
 };
 
-/// The kind of a completion entry.
+struct MarkedString {
+  /**
+ * MarkedString can be used to render human readable text. It is either a
+ * markdown string
+ * or a code-block that provides a language and a code snippet. The language
+ * identifier
+ * is sematically equal to the optional language identifier in fenced code
+ * blocks in GitHub
+ * issues. See
+ * https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting
+ *
+ * The pair of a language and a value is an equivalent to markdown:
+ * ```${language}
+ * ${value}
+ * ```
+ *
+ * Note that markdown 

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth added a comment.

I had missed a typo in the code, should be fixed and compiling properly now.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[William Enright via Phabricator via cfe-commits]

Nebiroth updated this revision to Diff 108979.
Nebiroth marked 19 inline comments as done.
Nebiroth added a comment.

Implemention of Code Hover as described in LSP definition.
Removed unintentional changes to formatting.
Removed file id field in Location struct.


https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/clients/clangd-vscode/src/extension.ts
  test/clangd/hover.test

Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,26 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int main() {\nint a;\na;\n}"}, "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int a"}, "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5
+
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -1,5 +1,6 @@
 import * as vscode from 'vscode';
 import * as vscodelc from 'vscode-languageclient';
+import * as vscodejsonrpc from 'vscode-jsonrpc';
 
 /**
  * Method to get workspace configuration option
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
 JSONOutput ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
 JSONOutput ) = 0;
+  virtual void onCodeHover(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput ) = 0;
 };
 
 void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,24 @@
   ProtocolCallbacks 
 };
 
+struct CodeHoverHandler : Handler {
+  CodeHoverHandler(JSONOutput , ProtocolCallbacks )
+  : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentPositionParams::parse(Params);
+if (!TDPP) {
+  Output.log("Failed to decode TextDocumentPositionParams!\n");
+  return;
+}
+
+Callbacks.onCodeHover(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks 
+};
+
 } // namespace
 
 void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +257,7 @@
   llvm::make_unique(Out, Callbacks));
   Dispatcher.registerHandler("textDocument/definition",
   llvm::make_unique(Out, Callbacks));
+  Dispatcher.registerHandler(
+  "textDocument/hover",
+  llvm::make_unique(Out, Callbacks));
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -23,6 +23,7 @@
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"
+#include "clang/Basic/SourceLocation.h"
 #include 
 #include 
 
@@ -304,7 +305,59 @@
   parse(llvm::yaml::MappingNode *Params);
 };
 
-/// The kind of a completion entry.
+struct MarkedString {
+  /**
+ * MarkedString can be used to render human readable text. It is either a
+ * markdown string
+ * or a code-block that provides a language and a code snippet. The language
+ * identifier
+ * is sematically equal to the optional language identifier in fenced code
+ * blocks in GitHub
+ * issues. See
+ * https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting
+ *
+ * The pair of a language and a value is an equivalent to markdown:
+ * ```${language}
+ *