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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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



================
Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}
----------------
We should call `flush()` before returning `Name` here. The `raw_string_ostream` is buffered.


================
Comment at: clangd/XRefs.cpp:373
+
+  return {};
+}
----------------
NIT: use `llvm::None` here instead of `{}`


================
Comment at: clangd/XRefs.cpp:394
+
+  //  SourceRange SR = D->getSourceRange();
+
----------------
Accidental leftover from previous code?


================
Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+    StringRef input;
+    StringRef expectedHover;
----------------
NIT: LLVM uses `UpperCamelCase` for field names.


================
Comment at: unittests/clangd/XRefsTests.cpp:561
+
+    EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }
----------------
simark wrote:
> Note that I used `.str()` here to make the output of failing tests readable and useful.  By default, gtest tries to print StringRef as if it was a container.  I tried to make add a `PrintTo` function to specify how it should be printed, but couldn't get it to work (to have it called by the compiler), so I settled for this.
Thanks for spotting that.
We have a fix for that in LLVM's gtest extensions (D43330).
`str()` can now be removed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894