Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: Add WriteStream::writeStream() #1998

Merged
merged 1 commit into from Feb 9, 2020

Conversation

ccawley2011
Copy link
Member

No description provided.

@dreammaster
Copy link
Member

That could indeed be a nice little helper. A few comments I have:

  1. It wouldn't hurt to return the bytes written as a result from the method, just like the write method does
  2. It may not hurt to have a second overloaded version of the method that simply takes in a SeekableReadStream and no size, and simply chains to the writeStream method with the stream and stream size. That way, callers that want to copy an entire stream don't have to explicitly specify the size, so long as it's a seekable one (which implements the size() method).

@@ -28,6 +28,14 @@

namespace Common {

void WriteStream::writeStream(ReadStream *stream, uint32 dataSize) {
void *buf = malloc(dataSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically data copying routines use some kind of buffer of a fixed size as temporary storage so that they can work on arbitrarily sized inputs. See what boost does for example:
https://github.com/boostorg/iostreams/blob/develop/include/boost/iostreams/copy.hpp#L123

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed nice to have, but I believe that it's outside of the scope of this pull request. The code can be adapted later on in-tree

@bluegr
Copy link
Member

bluegr commented Feb 9, 2020

This looks OK To merge now, thanks! Future enhancements, like @bgK's suggestion for arbitrarily sized inputs, can be done in-tree.

@bluegr bluegr merged commit 13e5042 into scummvm:master Feb 9, 2020
@ccawley2011 ccawley2011 deleted the writeStream branch February 9, 2020 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants