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

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.


================
Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}
----------------
ilya-biryukov wrote:
> We should call `flush()` before returning `Name` here. The `raw_string_ostream` is buffered.
I replaced it with `Stream.str()`.


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


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


================
Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+    StringRef input;
+    StringRef expectedHover;
----------------
ilya-biryukov wrote:
> NIT: LLVM uses `UpperCamelCase` for field names.
Ok, I fixed all the structs this patch adds.


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894