This is an archive of the discontinued LLVM Phabricator instance.

Added initial unit test for LLDB's Stream class.
ClosedPublic

Authored by teemperor on Jul 30 2018, 4:10 PM.

Details

Summary

This adds an initial small unit test for LLDB's Stream class, which should at least cover
most of the functions in the Stream class. StreamString is always in big endian
mode, so that's the only stream byte order path this test covers as of now. Also,
the binary mode still needs to be tested for all print methods.

Also adds some FIXMEs for wrong/strange result values of the Stream class that we hit
while testing those functions.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Jul 30 2018, 4:10 PM
teemperor updated this revision to Diff 158127.Jul 30 2018, 4:15 PM
  • Removed MaxHex64 test (which moved to the child revision to be in the same commit as the related bug).
labath accepted this revision.Jul 31 2018, 1:47 AM
labath added a subscriber: labath.

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.

unittests/Utility/StreamTest.cpp
38–41 ↗(On Diff #158127)

<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>

56 ↗(On Diff #158127)

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.

88–95 ↗(On Diff #158127)

Could you use raw string literals R"(...)" for the expectations? It's easier to see what this is doing that way.

This revision is now accepted and ready to land.Jul 31 2018, 1:47 AM
teemperor updated this revision to Diff 158411.Jul 31 2018, 4:03 PM
teemperor marked an inline comment as done.
  • Addressed Pavel's comments.

Will merge once I have time to watch the build bots afterwards.

unittests/Utility/StreamTest.cpp
38–41 ↗(On Diff #158127)

I support this notion. think most of LLDB's algorithms do not respect PDP ordering.

56 ↗(On Diff #158127)

Sounds good to me, makes it definitely more readable.

probinson added inline comments.
unittests/Utility/StreamTest.cpp
38–41 ↗(On Diff #158127)

PDP ordering? As in, PDP-11? I could imagine GDB had to support it many long ages ago, but AFAIK no such machine has been produced this century.

This revision was automatically updated to reflect the committed changes.
labath added inline comments.Aug 1 2018, 4:47 AM
unittests/Utility/StreamTest.cpp
38–41 ↗(On Diff #158127)

This millenium, even. :P

Ok, I'll see if I can dismantle it.