subject:"\[PATCH\] D38425\: \[clangd\] Document highlights for clangd"

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-12 Thread Phabricator via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL320474: [clangd] Document highlights for clangd (authored by 
ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D38425?vs=126371=126529#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38425

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/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.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/test/clangd/documenthighlight.test
  clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
  clang-tools-extra/trunk/test/clangd/initialize-params.test

Index: clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
===
--- clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
+++ clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
@@ -72,4 +72,6 @@
   Register("textDocument/rename", ::onRename);
   Register("workspace/didChangeWatchedFiles", ::onFileEvent);
   Register("workspace/executeCommand", ::onCommand);
+  Register("textDocument/documentHighlight",
+   ::onDocumentHighlight);
 }
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -509,6 +509,39 @@
   return llvm::None;
 }
 
+llvm::Expected>
+ClangdServer::findDocumentHighlights(PathRef File, Position Pos) {
+  auto FileContents = DraftMgr.getDraft(File);
+  if (!FileContents.Draft)
+return llvm::make_error(
+"findDocumentHighlights called on non-added file",
+llvm::errc::invalid_argument);
+
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+
+  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, , ,
+   this](ParsedAST *AST) {
+if (!AST) {
+  Err = llvm::make_error("Invalid AST",
+llvm::errc::invalid_argument);
+  return;
+}
+Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
+  });
+
+  if (Err)
+return std::move(*Err);
+  return make_tagged(Result, TaggedFS.Tag);
+}
+
 std::future ClangdServer::scheduleReparseAndDiags(
 PathRef File, VersionedDraft Contents, std::shared_ptr Resources,
 Tagged TaggedFS) {
Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -557,6 +557,36 @@
 };
 bool fromJSON(const json::Expr &, RenameParams &);
 
+enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
+
+/// A document highlight is a range inside a text document which deserves
+/// special attention. Usually a document highlight is visualized by changing
+/// the background color of its range.
+
+struct DocumentHighlight {
+
+  /// The range this highlight applies to.
+
+  Range range;
+
+  /// The highlight kind, default is DocumentHighlightKind.Text.
+
+  DocumentHighlightKind kind = DocumentHighlightKind::Text;
+
+  friend bool operator<(const DocumentHighlight ,
+const DocumentHighlight ) {
+int LHSKind = static_cast(LHS.kind);
+int RHSKind = static_cast(RHS.kind);
+return std::tie(LHS.range, LHSKind) < std::tie(RHS.range, RHSKind);
+  }
+
+  friend bool operator==(const DocumentHighlight ,
+ const DocumentHighlight ) {
+return LHS.kind == RHS.kind && LHS.range == RHS.range;
+  }
+};
+json::Expr toJSON(const DocumentHighlight );
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -57,6 +57,7 @@
  {"triggerCharacters", {"(", ","}},
  }},
 {"definitionProvider", true},
+{"documentHighlightProvider", true},
 {"renameProvider", true},
 {"executeCommandProvider",
  json::obj{
@@ -235,6 +236,22 @@
   C.reply(Result ? URI::fromFile(*Result).uri : "");
 }
 
+void ClangdLSPServer::onDocumentHighlight(Ctx

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-12 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM minor a slight tweak to fix compilation. I'll land this today.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-11 Thread William Enright via Phabricator via cfe-commits

Nebiroth updated this revision to Diff 126371.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
Herald added a subscriber: mgrang.

Merged with latest llvm/clang
Minor code cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}
\ No newline at end of file
Index:

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-08 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.




Comment at: clangd/ClangdUnit.cpp:249
+// 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());

Mentioning AST in this comment is weird, macros have nothing to do with the 
AST. We should remove/rephrase the comment.
I'm not sure if multiple occurences of `MacroInfo` are even possible here, but 
we could leave the code as is anyway.



Comment at: clangd/Protocol.h:562
+
+///
+/// A document highlight is a range inside a text document which deserves

NIT: remove this empty comment line and all the others.



Comment at: clangd/Protocol.h:581
+const DocumentHighlight ) {
+return std::tie(LHS.range) < std::tie(RHS.range) &&
+   std::tie(LHS.kind) < std::tie(RHS.kind);

This comparison does not provide a total order.
Please use `std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, 
int(RHS.kind))` instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-05 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle accepted this revision.
malaperle added a subscriber: sammccall.
malaperle added a comment.

This looks good to me. @ilya-biryukov, @sammccall  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/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-05 Thread William Enright via Phabricator via cfe-commits

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

Added static_cast when unparsing


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-05 Thread 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.

Sorry I forgot to submit this additional comment in my last review pass :(




Comment at: clangd/Protocol.cpp:365
+  {"range", toJSON(DH.range)},
+  {"kind", (int)DH.kind},
+  };

static_cast(DH.kind)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-05 Thread William Enright via Phabricator via cfe-commits

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

Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index:

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-04 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added a comment.

Can you also reformat the code too with clang-format? I think there's a few 
things in ClangdUnit.h/.cpp




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

We should do like findDefinitions here:
```
  if (!Resources)
return llvm::make_error(
"findDocumentHighlights called on non-added file",
llvm::errc::invalid_argument);
```



Comment at: clangd/ClangdServer.cpp:528
+if (!AST)
+  Err = llvm::make_error("Invalid AST",
+llvm::errc::invalid_argument);

Woops, I forgot the return here if it's an error, so it should be:
```
if (!AST) {
  Err = llvm::make_error("Invalid AST",
llvm::errc::invalid_argument);
  return;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-04 Thread William Enright via Phabricator via cfe-commits

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

Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are 
enabled


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index:

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-03 Thread 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/ClangdServer.cpp:528
+  Value = llvm::make_error(
+  "invalid AST",
+  llvm::errc::invalid_argument);

Invalid



Comment at: clangd/ClangdServer.cpp:530
+  llvm::errc::invalid_argument);
+Value = make_tagged(clangd::findDocumentHighlights(*AST, Pos, Logger), 
TaggedFS.Tag);
+  });

It trips here with assertions enabled because we haven't checked the previous 
"Value" for error before, sigh.
Expected must be checked before access or destruction.
Expected value was in success state. (Note: Expected values in success 
mode must still be checked prior to being destroyed).

How about this:
```
  std::vector Result;
  llvm::Optional Err;
  Resources->getAST().get()->runUnderLock([Pos, , ,
   this](ParsedAST *AST) {
if (!AST)
  Err = llvm::make_error("Invalid AST",
llvm::errc::invalid_argument);
Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
  });

  if (Err)
return std::move(*Err);

  return make_tagged(Result, TaggedFS.Tag);
```
The tests pass for me with this.



Comment at: clangd/Protocol.cpp:372
+
+
+

Too many new lines. Only keep one.



Comment at: clangd/Protocol.h:568
+struct DocumentHighlight {
+  /*
+   *

Use /// like other places



Comment at: clangd/Protocol.h:575
+  /**
+   * The highlight kind, default is DocumentHighlightKind.Text.
+   */

Use /// like other places


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-12-01 Thread William Enright via Phabricator via cfe-commits

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

Minor code cleanup
unparse and parse methods for JSON are updated


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-30 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added inline comments.



Comment at: clangd/ClangdServer.cpp:526
+if (!AST)
+  llvm::make_error(
+  "invalid AST",

missing return?
I get a warning that looks legit:
./tools/clang/tools/extra/clangd/ClangdServer.cpp:526:7: warning: ignoring 
return value of function declared with 'warn_unused_result' attribute 
[-Wunused-result]
  llvm::make_error(



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-30 Thread 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.

very minor comments




Comment at: clangd/ClangdServer.h:288
+  /// Get document highlights for a symbol hovered on.
+  llvm::Expected>
+  findDocumentHighlights(PathRef File, Position Pos);

"for a symbol hovered on."

It doesn't have to be a symbol and the user doesn't have to hover on it. So 
maybe just "for a given position"



Comment at: clangd/ClangdUnit.cpp:1088
+  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::None;



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

maybe move the check earlier? see comment above.



Comment at: clangd/Protocol.h:601
+
+/**
+ * A document highlight is a range inside a text document which deserves

Use /// like other structs


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-30 Thread William Enright via Phabricator via cfe-commits

Nebiroth updated this revision to Diff 124951.
Nebiroth marked 6 inline comments as done.
Nebiroth added a comment.

  Minor code cleanup
  getDeclarationLocation now returns llvm::Optional
  operator< for DocumentHighlight struct now properly compares the kind field


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:  "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-30 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdLSPServer.cpp:67
 );
+
   if (Params.rootUri && !Params.rootUri->file.empty())

malaperle wrote:
> extra line
Still there



Comment at: clangd/ClangdServer.cpp:513
+  auto FileContents = DraftMgr.getDraft(File);
+  assert(FileContents.Draft &&
+ "findDocumentHighlights is called for non-added document");

Other functions don't crash in this case anymore, we should follow the same 
pattern here (see `findDefinitions` for an example)



Comment at: clangd/ClangdServer.cpp:524
+if (!AST)
+  return;
+Result = clangd::findDocumentHighlights(*AST, Pos, Logger);

We should return an error this case.



Comment at: clangd/ClangdUnit.cpp:1062
+
+  DocumentHighlight getDocumentHighlight(SourceRange SR,
+ DocumentHighlightKind Kind) {

This function should be private.



Comment at: clangd/ClangdUnit.cpp:1096
+  Range R = {Begin, End};
+  Location L;
+  if (const FileEntry *F =

We should not return default-initialized `Locations` from this function.
Return `llvm::Optional` and handle the case when `getFileEntryForID` 
returns null by returning `llvm::None`.



Comment at: clangd/ClangdUnit.cpp:1165
 
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+ DeclMacrosFinder, IndexOpts);

Let's add a comment that we don't currently handle macros. It's ok for the 
first iteration, having `documentHighlights` for `Decl`s is useful enough to 
get it in.



Comment at: clangd/Protocol.h:621
+const DocumentHighlight ) {
+return std::tie(LHS.range) < std::tie(RHS.range);
+  }

Please also compare `kind` here, to make this operator consistent with 
`operator==`.
`std::tie(LHS.range, LHS.kind) < std::tie(RHS.range,  RHS.kind)` should work. 
If it doesn't, converting enums to int should help: `std::tie(LHS.range, 
int(LHS.kind)) < std::tie(RHS.range,  int(RHS.kind))`



Comment at: test/clangd/initialize-params-invalid.test:23
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {

NIT: Misindent



Comment at: test/clangd/initialize-params.test:23
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {

NIT: Misindent


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-29 Thread William Enright via Phabricator via cfe-commits

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

Fixed wrong content header making the test fail


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-29 Thread William Enright via Phabricator via cfe-commits

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

Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-28 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:246
+
+  C.reply(json::ary(Highlights->Value));
+}

malaperle wrote:
> I get a test failure here because there is an assertion that the Expected<> 
> needs to be checked. I can't really think of any failure case right now where 
> we wouldn't just return an empty array of highlights. But I think it's better 
> for consistency and future-proofing to keep the Expected<>.
> I think you can just do like in onRename for now
>   if (!Highlights) {
> C.replyError(ErrorCode::InternalError,
>  llvm::toString(Highlights.takeError()));
> return;
>   }
Just to be clear, the error I see in the output is:
Expected must be checked before access or destruction.
Expected value was in success state. (Note: Expected values in success 
mode must still be checked prior to being destroyed).

It asserts when in ClangdLSPServer::onDocumentHighlight, referencing 
Highlights->Value


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-27 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:246
+
+  C.reply(json::ary(Highlights->Value));
+}

I get a test failure here because there is an assertion that the Expected<> 
needs to be checked. I can't really think of any failure case right now where 
we wouldn't just return an empty array of highlights. But I think it's better 
for consistency and future-proofing to keep the Expected<>.
I think you can just do like in onRename for now
  if (!Highlights) {
C.replyError(ErrorCode::InternalError,
 llvm::toString(Highlights.takeError()));
return;
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-27 Thread William Enright via Phabricator via cfe-commits

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

Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-27 Thread 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.

I tested the patch and it works quite well! I think those are the last comments 
from me. Sorry, I should have bundled them together a bit more :(




Comment at: clangd/ClangdUnit.cpp:1019
+  const ASTContext 
+  Preprocessor 
+

Not used.



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

Last



Comment at: clangd/ClangdUnit.cpp:1069
+SourceLocation LocEnd =
+Lexer::getLocForEndOfToken(SR.getEnd(), 0, SourceMgr, LangOpts);
+Position Begin;

Don't call getLocForEndOfToken here again. This is the reason why sometimes it 
includes an extra character. Just to explain a bit, supposed we have
int Foo;

the first call to getLocForEnd in handleDeclOccurence will compute the position 
at
int Foo|;
Then if you call is again, it will go at the end of the ';' token so it will 
now be
int Foo;|



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-27 Thread William Enright via Phabricator via cfe-commits

Nebiroth updated this revision to Diff 124391.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Herald added a subscriber: klimek.

Removed temporary test file
Updated test to account for read-access symbol verification


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":20,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":20,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":23,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":13,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":20,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-27 Thread 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.

There were some things I missed, sorry about that!




Comment at: clangd/main.cpp:1
+#define MACRO 1
+namespace ns1 {

This files needs to be removed



Comment at: test/clangd/documenthighlight.test:10
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define
 MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() 
{\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint 
xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 
1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+

I think we should cover "kind":2 as well. It's probably not a big change.



Comment at: test/clangd/documenthighlight.test:15
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}

This comment should be updated, it was copy/pasted probably?



Comment at: test/clangd/documenthighlight.test:22
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":3,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}

This comment should be updated, it was copy/pasted probably?



Comment at: test/clangd/documenthighlight.test:29
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":1,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}

This comment should be updated, it was copy/pasted probably?


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-24 Thread William Enright via Phabricator via cfe-commits

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

Fixed test checking for values from an incorrect bit shift


https://reviews.llvm.org/D38425

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
  clangd/main.cpp
  test/clangd/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,38 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":3,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":1,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/main.cpp
===
--- /dev/null
+++ clangd/main.cpp
@@ -0,0 +1,22 @@
+#define MACRO 1
+namespace ns1 {
+struct MyClass {
+int xasd;
+void anotherOperation(){
+}
+static int foo(MyClass*) {
+return 0;
+}
+
+};
+struct Foo {
+int xasd;
+};
+}
+int main() {
+int bonjour;
+bonjour = 2;
+ns1::Foo bar = { xasd : 1};
+bar.xasd = 3;
+ns1::MyClass* Params;
+Params->anotherOperation();}
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -54,6 +54,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-24 Thread 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: test/clangd/documenthighlight.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.

we should test that we can get kind == 1, 2, 3



Comment at: test/clangd/documenthighlight.test:16
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+

kind 0 is invalid



Comment at: test/clangd/documenthighlight.test:23
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+

kind 216? 220?



Comment at: test/clangd/documenthighlight.test:29
+# Go to local variable
+# CHECK: 
{"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+

kind 0 is invalid


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-24 Thread William Enright via Phabricator via cfe-commits

Nebiroth updated this revision to Diff 124230.
Nebiroth marked 6 inline comments as done.
Nebiroth added a comment.

Fixed a few outstanding issues that were reported as completed


https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,37 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -54,6 +54,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;
   virtual void onRename(Ctx C, RenameParams ) = 0;
+  virtual void onDocumentHighlight(Ctx C,
+   TextDocumentPositionParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -74,4

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-24 Thread William Enright via Phabricator via cfe-commits

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

Getting DocumentHighlightKind is now done in DocumentHighlightsFinder
Removed duplicated and unused code
Refactored method and variable names


https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,37 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -54,6 +54,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;
   virtual void onRename(Ctx C, RenameParams ) = 0;
+  virtual void onDocumentHighlight(Ctx C,
+   TextDocumentPositionParams ) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-24 Thread William Enright via Phabricator via cfe-commits

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



Comment at: clangd/ClangdServer.h:291
+  /// Get document highlights for a symbol hovered on.
+  Tagged findDocumentHighlights(PathRef File,
+Position Pos);

malaperle wrote:
> can this thing fail? Should it be Expectedhttps://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-23 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added a comment.

In https://reviews.llvm.org/D38425#922408, @ioeric wrote:

> Drive-by comment: in general, have you considered reusing the existing 
> declarations and occurrences finding functionalities in clang-rename? AFAIK, 
> it deals with templates and macros pretty well.
>
> o 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
>  
>  o 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

This looks interesting. It does solve the problem that currently we traverse 
the whole tree. However, I don't think it has support for macros (they are not 
covered by USRs I believe) and it looks like it only can return one NamedDecl* 
at a location, and it's possible to have multiple, non-named Decls. Perhaps we 
should look at improving this!

https://reviews.llvm.org/D38425

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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-23 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:997
+  DocumentHighlightKind Kind;
+  switch (Roles) {
+  case (unsigned)index::SymbolRole::Read:

With this code, I always get "text" kind. It's because index::SymbolRoleSet is 
a bitfield so you have to check the write, read bits. Something like:

DocumentHighlightKind Kind = DocumentHighlightKind::Text;
if (static_cast(index::SymbolRole::Write) & Roles) {
  Kind = DocumentHighlightKind::Write;
} else if (static_cast(index::SymbolRole::Read) & 
Roles) {
  Kind = DocumentHighlightKind::Read;
}


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-23 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added inline comments.

Comment at: clangd/ClangdUnit.cpp:1245
+
+  for (unsigned I = 0; I < DocHighlightsFinder->getSourceRanges().size(); I++) 
{
+HighlightLocations.push_back(

malaperle wrote:
> replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? 
> then call takeHighlights.
Also, use a range-based for loop

Comment at: clangd/ClangdUnit.cpp:1247
+HighlightLocations.push_back(
+addHighlightLocation(AST, DocHighlightsFinder->getSourceRanges()[I],
+ DocHighlightsFinder->getKinds()[I]));

The vector returned by getKinds() doesn't match the size of getSourceRanges(). 
This is because the kinds vector is computed in DeclarationLocationsFinder 
which only computes the kind of the occurrence the user has selected, not the 
kinds of all occurrences for a given Decl. Once you move the code to create the 
highlights in DocumentHighlightsFinder, you won't need a vector of kinds and 
this won't be a problem.

https://reviews.llvm.org/D38425

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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-23 Thread 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:67
 );
+
   if (Params.rootUri && !Params.rootUri->file.empty())

extra line



Comment at: clangd/ClangdLSPServer.cpp:242
+
+  auto Items = Server
+   .findDocumentHighlights(Params.textDocument.uri.file,

Items -> Highlights?



Comment at: clangd/ClangdServer.h:291
+  /// Get document highlights for a symbol hovered on.
+  Tagged findDocumentHighlights(PathRef File,
+Position Pos);

can this thing fail? Should it be Expected DeclarationLocations;

I think this class should be exactly the same as the code hover patch, so 
changes from there and the comments on that patch should apply here too.



Comment at: clangd/ClangdUnit.cpp:944
   std::vector DeclarationLocations;
+  std::vector DeclarationDecls;
+  std::vector DeclarationMacroInfos;

just Decls?



Comment at: clangd/ClangdUnit.cpp:945
+  std::vector DeclarationDecls;
+  std::vector DeclarationMacroInfos;
+  std::vector Kinds;

just MacroInfos?



Comment at: clangd/ClangdUnit.cpp:946
+  std::vector DeclarationMacroInfos;
+  std::vector Kinds;
   const SourceLocation 

remove, see comment below.



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

Last



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

This comment needs to be updated (like the code hover patch)



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

Last



Comment at: clangd/ClangdUnit.cpp:992
   index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
+
 if (isSearchedLocation(FID, Offset)) {

extra line



Comment at: clangd/ClangdUnit.cpp:996
+  DeclarationDecls.push_back(D);
+  DocumentHighlightKind Kind;
+  switch (Roles) {

I think you need to do this in DocumentHighlightsFinder not here. Each 
occurrence can have its kind, not just the user selected location.



Comment at: clangd/ClangdUnit.cpp:1013
 
+  std::vector getDeclarationDecls() { return DeclarationDecls; };
+  std::vector getDeclarationMacroInfos() { return 
DeclarationMacroInfos; };

I don't think you need any of those getters. Only use the "takes"



Comment at: clangd/ClangdUnit.cpp:1079
 
+/// Finds document highlights that a given FileID and file offset refers to.
+class DocumentHighlightsFinder : public index::IndexDataConsumer {

It's not "a given FileID and file", the finder is given a list of Decls



Comment at: clangd/ClangdUnit.cpp:1122
 
+Location clangd::addDeclarationLocation(ParsedAST , SourceRange SR) {
+  const SourceManager  = AST.getASTContext().getSourceManager();

This is duplicated code with getDeclarationLocation. It also doesn't use 
tryGetRealPathName
I also don't think this is called??



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

I don't think this is called?



Comment at: clangd/ClangdUnit.cpp:1191
+
+DocumentHighlight clangd::addHighlightLocation(ParsedAST , SourceRange SR,
+   DocumentHighlightKind Kind) {

This doesn't add  or have a location in its signature so I think it should be 
named differently. 
DocumentHighligh getDocumentHighlight(Decl*) ? Or addDocumentHighlight(Decl*) 
if you move it in DocumentHighlightsFinder.



Comment at: clangd/ClangdUnit.cpp:1232
+
+  std::vector TempDecls = 
DeclLocationsFinder->getDeclarationDecls();
+  std::vector TempKinds =

call takeDecls. TempDecls -> SelectedDecls?



Comment at: clangd/ClangdUnit.cpp:1233
+  std::vector TempDecls = 
DeclLocationsFinder->getDeclarationDecls();
+  std::vector TempKinds =
+  DeclLocationsFinder->getKinds();

remove? see comment below



Comment at:

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-21 Thread William Enright via Phabricator via cfe-commits

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

Removed some commented lines and temporary code
Streamlined and removed some code that overlaps/conflicts with code hover patch 
so it's easier to merge (patch https://reviews.llvm.org/D35894)


https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,37 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -31,7 +31,6 @@
 public:
   using Ctx = RequestContext;
   virtual ~ProtocolCallbacks() = default;
-
   virtual void onInitialize(Ctx C, InitializeParams ) = 0;
   virtual void onShutdown(Ctx C, ShutdownParams ) = 0;
   virtual void onExit(Ctx C, ExitParams ) = 0;
@@ -54,6 +53,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams ) = 0;
   virtual void onRename(Ctx C, RenameParams ) = 0;
+

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-20 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added a comment.

Hi William, can you quickly go over the diff before I do a review? There are a 
few more-obvious things that can be addressed, i.e. commented lines, extra 
new/deleted lines, temporary code, lower case variable names, etc.


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-10 Thread William Enright via Phabricator via cfe-commits

Nebiroth added a comment.

This updated patch still does not handle highlighting macro references 
correctly. I will make another patch at a later time for this issue.

In https://reviews.llvm.org/D38425#922408, @ioeric wrote:

> Drive-by comment: in general, have you considered reusing the existing 
> declarations and occurrences finding functionalities in clang-rename? AFAIK, 
> it deals with templates and macros pretty well.
>
> o 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
>  
>  o 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

I'll take a look at it later. Thanks!

https://reviews.llvm.org/D38425

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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-10 Thread Eric Liu via Phabricator via cfe-commits

ioeric added a comment.

Drive-by comment: in general, have you considered reusing the existing 
declarations and occurrences finding functionalities in clang-rename? AFAIK, it 
deals with templates and macros pretty well.

o 
https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
 
o 
https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-10 Thread William Enright via Phabricator via cfe-commits

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

Decls and MacroInfos vectors are now private and passed by reference instead of 
copied.
DeclarationLocationsFinder does not store Locations anymore, instead the vector 
is filled in their respective methods in ClangdUnit.cpp
Refactored if condition
Removed redundant newlines at end of test file.


https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
@@ -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/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,37 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -31,7 +31,6 @@
 public:
   using Ctx = RequestContext;
   virtual ~ProtocolCallbacks() = default;
-
   virtual void onInitialize(Ctx C, InitializeParams ) = 0;
   virtual void onShutdown(Ctx C,

[PATCH] D38425: [clangd] Document highlights for clangd

2017-11-10 Thread William Enright via Phabricator via cfe-commits

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

- Decls and MacroInfos vectors are now private and passed by reference instead 
of copied.


https://reviews.llvm.org/D38425

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/documenthighlight.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
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -20,6 +20,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "definitionProvider": true,
 # CHECK-NEXT:  "documentFormattingProvider": true,
+# CHECK-NEXT:	   "documentHighlightProvider": true,
 # CHECK-NEXT:  "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:"firstTriggerCharacter": "}",
 # CHECK-NEXT:"moreTriggerCharacter": []
@@ -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/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,37 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":12,"line":16},"start":{"character":4,"line":16}}},{"kind":0,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}}]}
+
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":9,"line":12},"start":{"character":4,"line":12}}},{"kind":1,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":216,"range":{"end":{"character":21,"line":18},"start":{"character":17,"line":18}}},{"kind":220,"range":{"end":{"character":8,"line":19},"start":{"character":4,"line":19}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":22,"line":4},"start":{"character":5,"line":4}}},{"kind":0,"range":{"end":{"character":25,"line":21},"start":{"character":8,"line":21}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}			
\ No newline at end of file
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -31,7 +31,6 @@
 public:
   using Ctx = RequestContext;
   virtual ~ProtocolCallbacks() = default;
-
   virtual void onInitialize(Ctx C, InitializeParams ) = 0;
   virtual void onShutdown(Ctx C, ShutdownParams ) = 0;
   virtual void onExit(Ctx C, ExitParams ) = 0;
@@ -54,6 +53,8 @@
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(Ctx C,

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-23 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

> Another note: current implementation does not seem to handle macros at all 
> (it will only highlight definition of a macro, not its usages).

Current implementation still doesn't highlight macro references, only 
definitions. I'd either disable `documentHighlight` for macros for the initial 
version or come up with a way to implement `documentHighlight` for them.
I don't think `Preprocessor` currently stores information about all macro 
references, so we'll need to either store a separate index for that or rerun 
preprocessor to get this information.




Comment at: clangd/ClangdUnit.cpp:1017
+
+  auto DeclLocationsFinder = std::make_shared(
+  llvm::errs(), SourceLocationBeg, AST.getASTContext(),

Nebiroth wrote:
> ilya-biryukov wrote:
> > I wonder if we really need to rerun indexer twice here. Probably ok for the 
> > first version, but we should definitely think about a faster way to get the 
> > `Decl` under cursor than visiting the whole AST. Not sure if it's easy to 
> > do with clang's AST, though, maybe we'll need a separate index for that.
> Yeah I realise it's definitely not the fastest way to go about it. Maybe 
> there is a way to stop handling occurrences once the highlighted declaration 
> is found. This would be the most straightforward way of improving the 
> performance of the feature without re-writing a new AST indexer.
That only helps if we get lucky, average case and worst case would still be 
slow.
Anyway, this commit can certainly go in without this change.



Comment at: clangd/ClangdUnit.cpp:879
 public:
+  std::vector Decls;
+  std::vector MacroInfos;

Let's not store  `DeclarationLocations` anymore. If we have `Decl`s and 
`MacroInfo`s, we can move the code getting their `Location`s out of the 
`DeclarationLocationsFinder`.



Comment at: clangd/ClangdUnit.cpp:880
+  std::vector Decls;
+  std::vector MacroInfos;
   DeclarationLocationsFinder(raw_ostream ,

Please make the fields private, e.g. put them before `SearchedLocations`.



Comment at: clangd/ClangdUnit.cpp:976
+public:
+  std::vector Decls;
+  std::vector MacroInfos;

Make these fields private.
Don't copy `std::vector`s, store references to them instead.



Comment at: clangd/ClangdUnit.cpp:1003
+const SourceManager  = AST.getSourceManager();
+if (std::find(Decls.begin(), Decls.end(), D) != Decls.end()) {
+  SourceLocation Begin, End;

Please rewrite 
```
if (cond) {
  // long code
}
return true;
```
to 
```
if (!cond)
  return true;

// long code
```

This reduces nesting and makes code easier to read.
See [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | style guide ]].



Comment at: clangd/Protocol.h:431
+ */
+struct DocumentHighlight{
+  /**

ilya-biryukov wrote:
> There seems to be a missing brace before `{`. I generally run this simple 
> script before submission to make sure my code is always formatted:
> ```
> find "$CLANG_TOOLS_EXTRA_PATH/clangd" -name "*.cpp" -or -name "*.h" -exec 
> clang-format -i --style=LLVM {} \;
> find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" -name "*.cpp" -or -name "*.h" 
> -exec clang-format -i --style=LLVM {} \ ;
> ```
BTW, the script had an error. (missing some parens)
```
find "$CLANG_TOOLS_EXTRA_PATH/clangd" \( -name "*.cpp" -or -name "*.h" \) -exec 
clang-format -i --style=LLVM {} \;
find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" \( -name "*.cpp" -or -name 
"*.h" \) -exec clang-format -i --style=LLVM {} \ ;

```



Comment at: test/clangd/initialize-params.test:24
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+
+

Redundant newlines at the end of the file.


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-12 Thread William Enright via Phabricator via cfe-commits

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

Addressed review comments.
Fixed some tests not having updated providers.
Removed TargetDeclarationFinder for less code reuse.
DocumentHighlight struct is now unparsed correctly.


https://reviews.llvm.org/D38425

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/documenthighlight.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,18 +5,21 @@
 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: 580
 # 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:   "documentHighlightProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+
+
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: 580
 # 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:   "documentHighlightProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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"}}
+# CHECK: Content-Length: 580
+# 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:   "documentHighlightProvider": true
+# CHECK: }}}
+#
+
+Content-Length: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 16, "character": 4}, "end": {"line": 16, "character": 12}}, "kind": 1},{"range": {"start": {"line": 17, "character": 0}, "end": {"line": 17,

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-11 Thread William Enright via Phabricator via cfe-commits

Nebiroth marked an inline comment as done.
Nebiroth added inline comments.



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

ilya-biryukov wrote:
> This `IndexDataConsumer` effectively does the same thing as 
> `DeclarationLocationsFinder`.
> We should reuse the code between those two.
> 
> To do that we could:
> 1. Change `DeclarationLocationsFinder` to return a list of `Decl*`s and 
> `MacroDef*`s that are under caret.
> 2. In `findDefinition` we should return locations of found `Decl` and 
> `MacroDef`.
> 3. In `documentHighlights` we would rerun `DocumentHighlightsFinder`to 
> capture locations of `Decls` referenced in step 1.
> 
> Do you think this approach would work?
I have something similar to this implemented. Will upstream it soon.


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-11 Thread William Enright via Phabricator via cfe-commits

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



Comment at: clangd/ClangdUnit.cpp:1017
+
+  auto DeclLocationsFinder = std::make_shared(
+  llvm::errs(), SourceLocationBeg, AST.getASTContext(),

ilya-biryukov wrote:
> I wonder if we really need to rerun indexer twice here. Probably ok for the 
> first version, but we should definitely think about a faster way to get the 
> `Decl` under cursor than visiting the whole AST. Not sure if it's easy to do 
> with clang's AST, though, maybe we'll need a separate index for that.
Yeah I realise it's definitely not the fastest way to go about it. Maybe there 
is a way to stop handling occurrences once the highlighted declaration is 
found. This would be the most straightforward way of improving the performance 
of the feature without re-writing a new AST indexer.


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-10 Thread William Enright via Phabricator via cfe-commits

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

Rebased on master.


https://reviews.llvm.org/D38425

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/documenthighlight.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,18 +5,21 @@
 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: 580
 # 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:   "documentHighlightProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+
+
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: 580
 # 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:   "documentHighlightProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# 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"}}
+# CHECK: Content-Length: 580
+# 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:   "documentHighlightProvider": true
+# CHECK: }}}
+#
+
+Content-Length: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 16, "character": 4}, "end": {"line": 16, "character": 12}}, "kind": 1},{"range": {"start": {"line": 17, "character": 0}, "end": {"line": 17, "character": 7}}, "kind": 1}]}
+
+Content-Length: 157
+

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-06 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

Sorry for the delay.

The patch does not apply cleanly on top of the current head. Mostly conflicts 
with `switchHeaderSource` for me.
Could you please rebase your change with head and resubmit it?

Another note: current implementation does not seem to handle macros at all (it 
will only highlight definition of a macro, not its usages).
Do you have an idea on how to support them?




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

This `IndexDataConsumer` effectively does the same thing as 
`DeclarationLocationsFinder`.
We should reuse the code between those two.

To do that we could:
1. Change `DeclarationLocationsFinder` to return a list of `Decl*`s and 
`MacroDef*`s that are under caret.
2. In `findDefinition` we should return locations of found `Decl` and 
`MacroDef`.
3. In `documentHighlights` we would rerun `DocumentHighlightsFinder`to capture 
locations of `Decls` referenced in step 1.

Do you think this approach would work?



Comment at: clangd/ClangdUnit.cpp:1017
+
+  auto DeclLocationsFinder = std::make_shared(
+  llvm::errs(), SourceLocationBeg, AST.getASTContext(),

I wonder if we really need to rerun indexer twice here. Probably ok for the 
first version, but we should definitely think about a faster way to get the 
`Decl` under cursor than visiting the whole AST. Not sure if it's easy to do 
with clang's AST, though, maybe we'll need a separate index for that.



Comment at: clangd/Protocol.cpp:782
+llvm::raw_string_ostream(Result) << llvm::format(
+R"({"range": %s, "kind": %d})", Range::unparse(DH.range).c_str(), 
DH.kind);
+return Result;

It should work without `c_str()`, simply pass result of `unparse` (i.e., 
`std::string`) to `llvm::format`.
Generally, avoid calling `c_str()` (and `str()` on `StringRef` for that matter).



Comment at: clangd/Protocol.h:431
+ */
+struct DocumentHighlight{
+  /**

There seems to be a missing brace before `{`. I generally run this simple 
script before submission to make sure my code is always formatted:
```
find "$CLANG_TOOLS_EXTRA_PATH/clangd" -name "*.cpp" -or -name "*.h" -exec 
clang-format -i --style=LLVM {} \;
find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" -name "*.cpp" -or -name "*.h" 
-exec clang-format -i --style=LLVM {} \ ;
```


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-10-02 Thread William Enright via Phabricator via cfe-commits

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

Addressed initial comments.
Formatted ClangdUnit.h


https://reviews.llvm.org/D38425

Files:
  clangd/ClangdLSPServer.cpp
  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/documenthighlight.test

Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,29 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 16, "character": 4}, "end": {"line": 16, "character": 12}}, "number": 1},{"range": {"start": {"line": 17, "character": 0}, "end": {"line": 17, "character": 7}}, "number": 1}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 12, "character": 4}, "end": {"line": 12, "character": 9}}, "number": 1},{"range": {"start": {"line": 18, "character": 17}, "end": {"line": 18, "character": 21}}, "number": 1},{"range": {"start": {"line": 19, "character": 4}, "end": {"line": 19, "character": 8}}, "number": 1}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 4, "character": 5}, "end": {"line": 4, "character": 22}}, "number": 1},{"range": {"start": {"line": 21, "character": 8}, "end": {"line": 21, "character": 25}}, "number": 1}]}
+
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -47,7 +47,9 @@
   virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
 JSONOutput ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) = 0;
+JSONOutput ) = 0;
+  virtual void onDocumentHighlight(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 DocumentHighlightHandler : Handler {
+  DocumentHighlightHandler(JSONOutput , ProtocolCallbacks )
+  : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentPositionParams::parse(Params, Output);
+if (!TDPP) {
+  Output.log("Failed to decode TextDocumentPositionParams!\n");
+  return;
+}
+
+Callbacks.onDocumentHighlight(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks 
+};
+
 } // namespace
 
 void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -240,4 +258,7 @@
   Dispatcher.registerHandler(
   "textDocument/definition",
   llvm::make_unique(Out, Callbacks));
+  Dispatcher.registerHandler(
+  "textDocument/documentHighlight",
+  llvm::make_unique(Out, Callbacks));
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -416,6 +416,41 @@
   static std::string unparse(const CompletionItem );
 };
 
+enum class DocumentHighlightKind {
+  Text = 1,
+  Read = 2,
+  Write = 3
+};
+
+/**
+ * A document highlight is a range inside

[PATCH] D38425: [clangd] Document highlights for clangd

2017-09-29 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

Thanks for the patch, I'll take a closer look a bit later.
But just wanted to post one very important comment right away.




Comment at: clangd/ClangdUnit.h:268
+std::vector findDocumentHighlights(ParsedAST , Position 
Pos,
+  clangd::Logger );
+  

Please `clang-format` the code on every submission!


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-09-29 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added a comment.

Just a few quick comments.




Comment at: clangd/ClangdServer.cpp:295
+  assert(FileContents.Draft &&
+ "findDefinitions is called for non-added document");
+

findDocumentHighlights?



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

findDocumentHighlights?



Comment at: clangd/Protocol.cpp:782
+llvm::raw_string_ostream(Result) << llvm::format(
+R"({"range": %s, "number": %d})", Range::unparse(DH.range).c_str(), 
DH.kind);
+return Result;

number -> kind


https://reviews.llvm.org/D38425



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

[PATCH] D38425: [clangd] Document highlights for clangd

2017-09-29 Thread William Enright via Phabricator via cfe-commits

Nebiroth created this revision.

Implementation of Document Highlights Request as described in LSP.


https://reviews.llvm.org/D38425

Files:
  clangd/ClangdLSPServer.cpp
  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/documenthighlight.test

Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,29 @@
+# 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: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 16, "character": 4}, "end": {"line": 16, "character": 12}}, "number": 1},{"range": {"start": {"line": 17, "character": 0}, "end": {"line": 17, "character": 7}}, "number": 1}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 12, "character": 4}, "end": {"line": 12, "character": 9}}, "number": 1},{"range": {"start": {"line": 18, "character": 17}, "end": {"line": 18, "character": 21}}, "number": 1},{"range": {"start": {"line": 19, "character": 4}, "end": {"line": 19, "character": 8}}, "number": 1}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 4, "character": 5}, "end": {"line": 4, "character": 22}}, "number": 1},{"range": {"start": {"line": 21, "character": 8}, "end": {"line": 21, "character": 25}}, "number": 1}]}
+
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -47,7 +47,9 @@
   virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
 JSONOutput ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) = 0;
+JSONOutput ) = 0;
+  virtual void onDocumentHighlight(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 DocumentHighlightHandler : Handler {
+  DocumentHighlightHandler(JSONOutput , ProtocolCallbacks )
+  : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentPositionParams::parse(Params, Output);
+if (!TDPP) {
+  Output.log("Failed to decode TextDocumentPositionParams!\n");
+  return;
+}
+
+Callbacks.onDocumentHighlight(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks 
+};
+
 } // namespace
 
 void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -240,4 +258,7 @@
   Dispatcher.registerHandler(
   "textDocument/definition",
   llvm::make_unique(Out, Callbacks));
+  Dispatcher.registerHandler(
+  "textDocument/documentHighlight",
+  llvm::make_unique(Out, Callbacks));
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -416,6 +416,41 @@
   static std::string unparse(const CompletionItem );
 };
 
+enum class DocumentHighlightKind {
+  Text = 1,
+  Read = 2,
+  Write = 3
+};
+
+/**
+ * A document highlight is a range inside a text document which deserves
+ * special attention. Usually a

49 matches

Mail list logo