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

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

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

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



================
Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"
----------------
I don't know if the intention was to keep the protocol completely out of the ClangdServer class. Maybe someone else can clarify. But either way, I see that ClangdUnit does include "Protocol.h" so it's probably OK for now.


================
Comment at: clangd/ClangdServer.cpp:297
+  
+  MarkedString MS = MarkedString("", "");
+  Range R;
----------------
Use default constructor for MarkedString instead? MarkedString MS;


================
Comment at: clangd/ClangdUnit.h:72
 
+  Hover getHover(Location L){
+
----------------
Move definition to ClangdUnit.cpp ?


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


================
Comment at: clangd/ClangdUnit.h:76
+    Range R;
+    const FileEntry *FE = Unit->getFileManager().getFile(L.uri.file);
+    FileID id = Unit->getSourceManager().translateFile(FE);
----------------
I don't know if you have access to a FileManager with the latest SVN trunk.


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


================
Comment at: clangd/ClangdUnit.h:87
+  }
+  unsigned start;
+  unsigned end;
----------------
I don't think those two are used (start,end). But if they are, those fields should not be in ClangdUnit.h


================
Comment at: clangd/Protocol.h:26
 #include "llvm/Support/YAMLParser.h"
+#include "clang/Basic/SourceLocation.h"
 #include <string>
----------------
not needed?


================
Comment at: clangd/Protocol.h:309
+struct MarkedString {
+  /**
+ * MarkedString can be used to render human readable text. It is either a
----------------
I don't think we should copy big parts of the documentation verbatim, otherwise, we'd have to distribute part of Clangd under the license for the LSP. I'm not familiar with "Creative Commons Attribution 3.0", etc to really know if you can copy everything and change the license to the LLVM's but I think it's safe to say that we can't.


================
Comment at: clangd/Protocol.h:329
+  MarkedString(std::string markdown)
+      : markdownString(markdown), codeBlockLanguage(""), codeBlockValue("") {}
+
----------------
I don't think the constructors bring much, I think they should be removed.


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


================
Comment at: clangd/Protocol.h:344
+
+  Hover(MarkedString ms, Range r) : contents(ms), range(r) {}
+
----------------
MS, R  (first letters of params/local vars is capitalized)


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


https://reviews.llvm.org/D35894