Mojo C bindings library pt1: MojomBuffer

Mojo C bindings library pt1: MojomBuffer

Also, make MOJO_STATIC_ASSERT work in C.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/fd8d18dbf7a4f9ed8a607648cca6d2c595b1c580

[vardhan, 2016-06-15 08:06:06]

PTAL

[viettrungluu, 2016-06-15 15:00:58]

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
File mojo/public/c/bindings/buffer.h (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
mojo/public/c/bindings/buffer.h:14: // |MojomBuffer| is an object that manages a
buffer. It is used as a
nit: "object" and "manages" are both stretches. Probably just say that it's used
to track buffer state during mojom serialization.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
mojo/public/c/bindings/buffer.h:26: // Allocates |num_bytes| (aligned to 8
bytes) from |buf|.
nit: "aligned" -> "rounded up to a multiple of", since there's no requirement
that |buf->buf| be aligned.

You should also say that it returns null on failure (and maybe clarify that this
occurs if there's not enough space).

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
File mojo/public/c/bindings/lib/buffer.c (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:16: const size_t size = (num_bytes +
(kAlignment-1)) & ~(kAlignment-1);
Note that this will lead to sadness if num_bytes is very large (so that
num_bytes + (kAlignment-1) overflows).

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:17: if (bytes_used + size > buf->buf_size ||
bytes_used + size <= bytes_used)
nit: <= -> <. There's no reason to not allow "allocations" of 0 bytes to
succeed.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
File mojo/public/c/bindings/tests/buffer_unittest.cc (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
nit: blank line above

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:23: }
You should test that allocating things that are already a multiple of 8 in size
don't result in larger allocations.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:25: TEST(MojomBufferTest,
Failure) {
You should really test the subtle part, namely the overflow logic.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:34: // Allocating 0 bytes should
fail as well.
Why should it necessarily be a failure?

[vardhan, 2016-06-15 15:48:00]

ptal

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
File mojo/public/c/bindings/buffer.h (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
mojo/public/c/bindings/buffer.h:14: // |MojomBuffer| is an object that manages a
buffer. It is used as a
On 2016/06/15 15:00:57, viettrungluu wrote:
> nit: "object" and "manages" are both stretches. Probably just say that it's
used
> to track buffer state during mojom serialization.

Done.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/buff...
mojo/public/c/bindings/buffer.h:26: // Allocates |num_bytes| (aligned to 8
bytes) from |buf|.
On 2016/06/15 15:00:57, viettrungluu wrote:
> nit: "aligned" -> "rounded up to a multiple of", since there's no requirement
> that |buf->buf| be aligned.
> 
> You should also say that it returns null on failure (and maybe clarify that
this
> occurs if there's not enough space).

Done.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
File mojo/public/c/bindings/lib/buffer.c (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:16: const size_t size = (num_bytes +
(kAlignment-1)) & ~(kAlignment-1);
On 2016/06/15 15:00:58, viettrungluu wrote:
> Note that this will lead to sadness if num_bytes is very large (so that
> num_bytes + (kAlignment-1) overflows).

oof.  i think its better to make it a uint32_t rather than a size_t (makes the
limit more clear, and is also more accurate).

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:17: if (bytes_used + size > buf->buf_size ||
bytes_used + size <= bytes_used)
On 2016/06/15 15:00:57, viettrungluu wrote:
> nit: <= -> <. There's no reason to not allow "allocations" of 0 bytes to
> succeed.

Allocations of 0 seem like a programmatic error -- if i return a non-null
pointer, the user shouldn't use it, so whats the point?  we also don't have a
free(), so there's nothing they could do with it, so I was thinking lets just
not allow it?

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
File mojo/public/c/bindings/tests/buffer_unittest.cc (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
On 2016/06/15 15:00:58, viettrungluu wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:23: }
On 2016/06/15 15:00:58, viettrungluu wrote:
> You should test that allocating things that are already a multiple of 8 in
size
> don't result in larger allocations.

Done.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:25: TEST(MojomBufferTest,
Failure) {
On 2016/06/15 15:00:58, viettrungluu wrote:
> You should really test the subtle part, namely the overflow logic.

Done.

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/test...
mojo/public/c/bindings/tests/buffer_unittest.cc:34: // Allocating 0 bytes should
fail as well.
On 2016/06/15 15:00:58, viettrungluu wrote:
> Why should it necessarily be a failure?

(see other comment)

[viettrungluu, 2016-06-15 15:59:49]

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
File mojo/public/c/bindings/lib/buffer.c (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:17: if (bytes_used + size > buf->buf_size ||
bytes_used + size <= bytes_used)
On 2016/06/15 15:48:00, vardhan wrote:
> On 2016/06/15 15:00:57, viettrungluu wrote:
> > nit: <= -> <. There's no reason to not allow "allocations" of 0 bytes to
> > succeed.
> 
> Allocations of 0 seem like a programmatic error -- if i return a non-null
> pointer, the user shouldn't use it, so whats the point?  we also don't have a
> free(), so there's nothing they could do with it, so I was thinking lets just
> not allow it?

It's not necessarily a programmatic error for cases when num_bytes is computed;
it can often simplify logic.

For comparison, it's often convenient to memcpy() or memset() 0 bytes. (It's
extremely inconvenient that the standard doesn't actually allow the pointers to
be null in those cases, though that's nearly never stopped anyone from doing
it.)

https://codereview.chromium.org/2062323002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/tests/buffer_unittest.cc (right):

https://codereview.chromium.org/2062323002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/tests/buffer_unittest.cc:42: mbuf.num_bytes_used =
UINT32_MAX - 7;  // This divides by 8.
nit: You should probably explicitly include <stdint.h> for UINT32_MAX.

[vardhan, 2016-06-15 16:10:21]

ptal

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
File mojo/public/c/bindings/lib/buffer.c (right):

https://codereview.chromium.org/2062323002/diff/1/mojo/public/c/bindings/lib/...
mojo/public/c/bindings/lib/buffer.c:17: if (bytes_used + size > buf->buf_size ||
bytes_used + size <= bytes_used)
On 2016/06/15 15:59:49, viettrungluu wrote:
> On 2016/06/15 15:48:00, vardhan wrote:
> > On 2016/06/15 15:00:57, viettrungluu wrote:
> > > nit: <= -> <. There's no reason to not allow "allocations" of 0 bytes to
> > > succeed.
> > 
> > Allocations of 0 seem like a programmatic error -- if i return a non-null
> > pointer, the user shouldn't use it, so whats the point?  we also don't have
a
> > free(), so there's nothing they could do with it, so I was thinking lets
just
> > not allow it?
> 
> It's not necessarily a programmatic error for cases when num_bytes is
computed;
> it can often simplify logic.
> 
> For comparison, it's often convenient to memcpy() or memset() 0 bytes. (It's
> extremely inconvenient that the standard doesn't actually allow the pointers
to
> be null in those cases, though that's nearly never stopped anyone from doing
> it.)

Done.

https://codereview.chromium.org/2062323002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/tests/buffer_unittest.cc (right):

https://codereview.chromium.org/2062323002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/tests/buffer_unittest.cc:42: mbuf.num_bytes_used =
UINT32_MAX - 7;  // This divides by 8.
On 2016/06/15 15:59:49, viettrungluu wrote:
> nit: You should probably explicitly include <stdint.h> for UINT32_MAX.

Done.

[viettrungluu, 2016-06-15 16:14:24]

lgtm

[vardhan, 2016-06-15 16:19:06]

Description was changed from

==========
Mojo C bindings library pt1: MojomBuffer

Also, make MOJO_STATIC_ASSERT work in C.

R=viettrungluu@chromium.org
==========

to

==========
Mojo C bindings library pt1: MojomBuffer

Also, make MOJO_STATIC_ASSERT work in C.

R=viettrungluu@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/fd8d18dbf7a4f9ed8a607648cca...
==========

[vardhan, 2016-06-15 16:19:08]

Committed patchset #3 (id:40001) manually as
fd8d18dbf7a4f9ed8a607648cca6d2c595b1c580 (presubmit successful).