search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Add Read Null Terminated String #313

Closed Brett208 closed 4 years ago

Brett208 commented 4 years ago

Closes #311

I think tests are fairly robust, although they are not easy to read

Brett208 commented 4 years ago

@DanRStevens I think this addresses the points you brought up??? It is definitely better written now.

Looks like I accidentally committed readme changes into this branch. Out of time to deal with it right now.

DanRStevens commented 4 years ago

I went and split out the ReadMe.md updates into PR #316.

I did a bit of work on the unit tests to hopefully make them a little easier to read, and to test a bit more in terms of stream position expectations.

After looking at the unit tests more closely, I wonder if we might change the specification for when the end of the stream has been reached. Maybe we should return the string read so far, rather than throwing an exception? This is of course a bit of an arbitrary choice. One interesting case is it might make sense to throw an exception if you were already at the end of the stream, rather than silently return a string of length 0. Or maybe a string of length 0 is entirely desirable.

If we make a change to not throw, but rather return the string read so far, we could consider using the non-throwing ReadPartial method internally.

Edit: If you want to stick with the throwing version, this should be good to merge.