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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
