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