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

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

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

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


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



================
Comment at: clangd/ClangdLSPServer.cpp:331
+                     if (!H) {
+                       replyError(ErrorCode::InternalError,
+                                  llvm::toString(H.takeError()));
----------------
ilya-biryukov wrote:
> NIT: use `return replyError` to be consistent with other methods.
Ok.


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


================
Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+    StringRef input;
+    StringRef expectedHover;
----------------
ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > NIT: LLVM uses `UpperCamelCase` for field names.
> > Ok, I fixed all the structs this patch adds.
> Argh, sorry that I didn't mention it before, but we have an exception for structs in `Procotol.h`, they follow the spelling in the LSP spec (i.e. should be `lowerCamelCase`).
> 
Haha, no problem, changing them back is one keystroke away.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894