[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 31 01:47:44 PDT 2018
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
This is really great. Thank you for doing this. I have some small ideas for improvement, but I don't think we have to go through another review cycle for that.
================
Comment at: unittests/Utility/StreamTest.cpp:38-41
+TEST_F(StreamTest, ChangingByteOrder) {
+ s.SetByteOrder(lldb::eByteOrderPDP);
+ EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}
----------------
<musing> I've been wondering for a while whether we shouldn't just remove PDP byte order support. Most of our code doesn't really support it, and neither does llvm's, so this is kind of a prerequisite for switching to llvm streams. </musing>
================
Comment at: unittests/Utility/StreamTest.cpp:56
+ s.PutChar('\n');
+ EXPECT_EQ(" \n", Value());
+
----------------
How do you feel about changing `Value` to call `Clear` on the underlying StreamString after fetching the string (and maybe renaming it to `TakeValue` or something)? That way, you could easily test the string printed by a specific function, instead of having to accumulate the expectations.
================
Comment at: unittests/Utility/StreamTest.cpp:88-95
+ s.QuotedCString("foo");
+ EXPECT_EQ("\"foo\"", Value());
+
+ s.QuotedCString("bar");
+ EXPECT_EQ("\"foo\"\"bar\"", Value());
+
+ s.QuotedCString(" ");
----------------
Could you use raw string literals `R"(...)"` for the expectations? It's easier to see what this is doing that way.
https://reviews.llvm.org/D50027
More information about the lldb-commits
mailing list