Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74

[Richard Wilbur]

I'm highly in favour of avoiding malloc churn!  However, I've got a
beef with Buffers::copy() below:

On Sun, Jun 15, 2014 at 1:56 PM, Bastiaan Jacques <address@hidden> wrote:
> diff --git a/libsound/LiveSound.h b/libsound/LiveSound.h
> index 82df6a5..8551297 100644
> --- a/libsound/LiveSound.h
> +++ b/libsound/LiveSound.h
[...]
> +    /// Copy up to the given number of bytes to the given buffer.
> +    //
> +    /// @to points to a buffer to be written to.
> +    /// @bytes number of bytes to be written.
> +    /// @return number of bytes actually written.
> +    size_t copy(std::uint8_t* to, size_t bytes) {
> +        assert(_consumed >= _in_point);
> +
> +        size_t bytes_remaining = bytes;
> +
> +        for (; _index < _buffers.size(); ++_index) {
> +            const SimpleBuffer& buffer = _buffers[_index];
> +
> +            size_t to_copy = std::min(bytes_remaining, buffer.size() - _pos);
> +
> +            std::copy(buffer.data() + _pos, buffer.data() + _pos + to_copy, 
> to);
> +            to += to_copy;
> +            bytes_remaining -= to_copy;
> +            _pos += to_copy;
> +
> +            if (_pos == buffer.size()) {
> +                ++_index;
> +                _pos = 0;
> +                break;
> +            }
> +
> +            if (bytes_remaining == 0) {
> +                break;
> +            }
> +        }
> +
> +        size_t written = bytes - bytes_remaining;
> +        _consumed += written;
> +        return written;
> +    }
> +

I would recommend changing the for() loop to a while(_index <
_buffers.size()) and removing the break statement from if (_pos ==
buffer.size()).  When _pos == buffer.size(), it means you just
finished copying the present buffer and need to update _index to point
to the next buffer and _pos to start at its beginning, but it doesn't
necessarily mean you are done copying unless bytes_remaining == 0,
right?  But if we have the for() loop also incrementing _index, it
seems to me we'll consistently skip buffers.

Suggested wording below:

+        while (_index < _buffers.size()) {
+            const SimpleBuffer& buffer = _buffers[_index];
+
+            size_t to_copy = std::min(bytes_remaining, buffer.size() - _pos);
+
+            std::copy(buffer.data() + _pos, buffer.data() + _pos +
to_copy, to);
+            to += to_copy;
+            bytes_remaining -= to_copy;
+            _pos += to_copy;
+
+            if (_pos == buffer.size()) {
+                ++_index;
+                _pos = 0;
+            }
+
+            if (bytes_remaining == 0) {
+                break;
+            }
+        }