Mojo C++ bindings

Mojo C++ bindings

This CL includes code for a static library that supports C++ bindings to the Mojo message format. A sample interface with "generated" bindings is included in the sample/ folder along with sample_test.cc that shows how the generated code may be used.

This is a WIP but seems worth snapshotting as is. See the TODO file for a list of near term improvements, most notably testing.

Originally reviewed at https://codereview.chromium.org/23913008/

TBR=viettrungluu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228193

[commit-bot: I haz the power, 2013-10-11 14:42:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/27034003/2001

[commit-bot: I haz the power, 2013-10-11 15:47:55]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[darin (slow to review), 2013-10-11 15:59:47]

Committed patchset #2 manually as r228193 (presubmit successful).

[dmichael (off chromium), 2013-10-14 22:41:19]

Random late ignorant driveby comments.

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
File mojo/public/bindings/lib/bindings.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/bindings.h:23: size_t num_bytes = sizeof(Array<T>) +
sizeof(StorageType) * num_elements;
Do you want to require here that num_bytes is a multiple of 8 bytes? Do you want
each element to be on an 8 byte boundary, or is it OK in your design to have
4-byte aligned types land on 4-byte boundaries?

I think you're OK, but I wanted to make sure you'd thought about it whether you
need to stick an Align() somewhere in the calculation of num_bytes.

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/bindings.h:63: return
reinterpret_cast<StorageType*>(this + 1);
Maybe you've already thought about this, but my understanding was that the
result of this conversion is unspecified. (See C++11 standard, 5.2.10p7). I
suspect it works in practice right now.

But note that your header is 4-byte aligned and 8-bytes wide. If T is a type
that has to be 8-byte aligned, then it probably works in practice right now. But
I think if your header changed to e.g. a 12-byte wide, 4-byte-aligned struct, I
think this would do the wrong thing. I.e., storage()[0] would not be aligned to
the 8-byte requirements of T. You might be able to mitigate that with a static
assert that the header size is a multiple of 8.

It also seems like doing it this way might obfuscate your code enough that the
optimizer won't do as well for at/operator[] as if you simply had the header and
an array as members. I think what you might really want is a zero-length array
after header_:
http://gcc.gnu.org/onlinedocs/gcc-4.0.4/gcc/Zero-Length.html
I think that solves a bunch of my above concerns. Alignment should be right, and
storage(), at(), and friends can be implemented more naturally.

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
File mojo/public/bindings/lib/bindings_serialization.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/bindings_serialization.h:123: }
Would it be better to simply leave all of these functions undefined (or
omitted)? That way, if somebody tries to specialize ArrayHelper with an
unsupported type, they get a compile or link error instead of a runtime error.

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/bindings_serialization.h:156: result +=
ComputeAlignedSizeOf(elements[i].ptr);
Sorry if this is a dumb question, but it looks like ComputeAlignedSizeOf will
return 0 if elements[i].ptr is NULL. Is that really the right thing?

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
File mojo/public/bindings/lib/buffer.cc (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/buffer.cc:61: Segment* segment =
static_cast<Segment*>(malloc(sizeof(Segment) + delta));
On 32-bit, pointers may be 4 bytes, so |Segment| is 24 bytes wide. I think in
that case |segment| is not aligned to 8 bytes. Did I miss something?

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
File mojo/public/bindings/lib/buffer.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/b...
mojo/public/bindings/lib/buffer.h:80: // memory is zero-filled.
tiny nit, singular/plural disagreement. Maybe:
contents...are
or
content...is

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/m...
File mojo/public/bindings/lib/message.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/m...
mojo/public/bindings/lib/message.h:34: // The receiver may mutate the given
message or take ownership of the its
"the its"->"its"

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/m...
mojo/public/bindings/lib/message.h:36: // regains ownership of the Message and
should be responsible for freeing its
s/regains/retains?
s/should be/is?

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/m...
File mojo/public/bindings/lib/message_builder.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/m...
mojo/public/bindings/lib/message_builder.h:29: FixedBuffer buf_;
nit: MOJO_DISALLOW_COPY_AND_ASSIGN?

(just for self-documenting; it's already not copyable because |buf_| isn't)

https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros.h
File mojo/public/system/macros.h (right):

https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros....
mojo/public/system/macros.h:18: #elif defined(__clang__)
nit: Could just use:
#if defined(_MSV_VER) || defined(__clang__)

https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros....
mojo/public/system/macros.h:33: typedef internal::CompileAssert<(bool(expr))>
msg[bool(expr) ? 1 : -1]
nit: This is kind of a weird mix of static assert techniques to me. What's the
point of CompileAssert? It seems like you could juse use any fundamental type
like char and get the same effect in a way that's C-compatible:
typedef char msg[bool(expr) ? 1 : -1];

The kind of canonical C++ way is to do:
template <bool value>
struct CompileAssert;

template <>
struct CompileAssert<true> {};

(I.e., only provide a "true" specialization CompileAssert so that a false will
cause a compile error that there's no valid specialization for "false".)

In the code here, I don't see the purpose of the CompileAssert struct.

[darin (slow to review), 2013-10-15 07:50:26]

On Mon, Oct 14, 2013 at 3:41 PM, <dmichael@chromium.org> wrote:

> Random late ignorant driveby comments.
>
>
Thank you for the feedback!


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings.h>
> File mojo/public/bindings/lib/**bindings.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings.**h#newcode23<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings.h#newcode23>
> mojo/public/bindings/lib/**bindings.h:23: size_t num_bytes =
> sizeof(Array<T>) + sizeof(StorageType) * num_elements;
> Do you want to require here that num_bytes is a multiple of 8 bytes? Do
> you want each element to be on an 8 byte boundary, or is it OK in your
> design to have 4-byte aligned types land on 4-byte boundaries?
>
> I think you're OK, but I wanted to make sure you'd thought about it
> whether you need to stick an Align() somewhere in the calculation of
> num_bytes.
>

The intent is to allow elements to be packed. It may make sense to insert
some padding into the array itself. I've been toying with doing that for
structs as well. Some things would get simpler if each object's size was a
multiple of 8 bytes. (Objects, arrays and structs, are allocated on 8-byte
boundaries, but their sizes are not required to be multiples of 8.)



>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings.**h#newcode63<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings.h#newcode63>
> mojo/public/bindings/lib/**bindings.h:63: return
> reinterpret_cast<StorageType*>**(this + 1);
> Maybe you've already thought about this, but my understanding was that
> the result of this conversion is unspecified. (See C++11 standard,
> 5.2.10p7). I suspect it works in practice right now.
>

I see. I suppose I could do some casting then. I was just using this as
shorthand for:
reinterpret_cast<StorageType*>(reinterpret_cast<char*>(this) +
sizeof(*this))

I also intend to go back and sprinkle in some compile assertions for the
sizes of
various types.



>
> But note that your header is 4-byte aligned and 8-bytes wide.


Allocations made via the Buffer class are always 8-byte aligned.



> If T is a
> type that has to be 8-byte aligned, then it probably works in practice
> right now. But I think if your header changed to e.g. a 12-byte wide,
> 4-byte-aligned struct, I think this would do the wrong thing. I.e.,
> storage()[0] would not be aligned to the 8-byte requirements of T. You
> might be able to mitigate that with a static assert that the header size
> is a multiple of 8.
>

Right. The header needs to be a multiple of 8. For now, I would just assert
that it is exactly 8.



>
> It also seems like doing it this way might obfuscate your code enough
> that the optimizer won't do as well for at/operator[] as if you simply
> had the header and an array as members. I think what you might really
> want is a zero-length array after header_:
>
http://gcc.gnu.org/onlinedocs/**gcc-4.0.4/gcc/Zero-Length.html<http://gcc.gnu...
> I think that solves a bunch of my above concerns. Alignment should be
> right, and storage(), at(), and friends can be implemented more
> naturally.
>

I was using something like that before, actually a 1-length array. I wonder
if zero-length
is permitted by the standard. I also wonder what it does to the size of an
object.

Again, my plan is to compile assert the size of these structs. The array
header will
need to be exactly 8 bytes. Mojo IPC will be prescriptive about such things
since
the goal is to invent a binary stable encoding.


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings_**serialization.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h>
> File mojo/public/bindings/lib/**bindings_serialization.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings_**serialization.h#newcode123<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h#newcode123>
> mojo/public/bindings/lib/**bindings_serialization.h:123: }
> Would it be better to simply leave all of these functions undefined (or
> omitted)? That way, if somebody tries to specialize ArrayHelper with an
> unsupported type, they get a compile or link error instead of a runtime
> error.
>

Hmm, I think this template is used for primitive types: e.g.,
mojo::Array<int32_t>.


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/bindings_**serialization.h#newcode156<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h#newcode156>
> mojo/public/bindings/lib/**bindings_serialization.h:156: result +=
> ComputeAlignedSizeOf(elements[**i].ptr);
> Sorry if this is a dumb question, but it looks like ComputeAlignedSizeOf
> will return 0 if elements[i].ptr is NULL. Is that really the right
> thing?
>
>
Yes. Arrays of objects are represented as arrays of pointers. This
particular
function is only responsible for accumulating the number of bytes pointed to
by elements of the array. The array storage itself is accounted for
elsewhere.



> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/buffer.cc<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/buffer.cc>
> File mojo/public/bindings/lib/**buffer.cc (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/buffer.cc#**newcode61<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/buffer.cc#newcode61>
> mojo/public/bindings/lib/**buffer.cc:61: Segment* segment =
> static_cast<Segment*>(malloc(**sizeof(Segment) + delta));
> On 32-bit, pointers may be 4 bytes, so |Segment| is 24 bytes wide. I
> think in that case |segment| is not aligned to 8 bytes. Did I miss
> something?
>

I think you meant 12 instead of 24, and yeah... good catch!


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/buffer.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/buffer.h>
> File mojo/public/bindings/lib/**buffer.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/buffer.h#**newcode80<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/buffer.h#newcode80>
> mojo/public/bindings/lib/**buffer.h:80: // memory is zero-filled.
> tiny nit, singular/plural disagreement. Maybe:
> contents...are
> or
> content...is
>

Thanks


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/message.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/message.h>
> File mojo/public/bindings/lib/**message.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/message.h#**newcode34<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/message.h#newcode34>
> mojo/public/bindings/lib/**message.h:34: // The receiver may mutate the
> given message or take ownership of the its
> "the its"->"its"
>

Thanks


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/message.h#**newcode36<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/message.h#newcode36>
> mojo/public/bindings/lib/**message.h:36: // regains ownership of the
> Message and should be responsible for freeing its
> s/regains/retains?
> s/should be/is?
>
>
Thanks



> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/message_**builder.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/message_builder.h>
> File mojo/public/bindings/lib/**message_builder.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/bindings/lib/message_**builder.h#newcode29<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/message_builder.h#newcode29>
> mojo/public/bindings/lib/**message_builder.h:29: FixedBuffer buf_;
> nit: MOJO_DISALLOW_COPY_AND_ASSIGN?
>
> (just for self-documenting; it's already not copyable because |buf_|
> isn't)
>

True


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/system/macros.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros.h>
> File mojo/public/system/macros.h (right):
>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/system/macros.h#**newcode18<https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros.h#newcode18>
> mojo/public/system/macros.h:**18: #elif defined(__clang__)
> nit: Could just use:
> #if defined(_MSV_VER) || defined(__clang__)
>

Thanks. This is just duplicated from base/compiler_specific.h


>
> https://codereview.chromium.**org/27034003/diff/2001/mojo/**
>
public/system/macros.h#**newcode33<https://codereview.chromium.org/27034003/diff/2001/mojo/public/system/macros.h#newcode33>
> mojo/public/system/macros.h:**33: typedef
> internal::CompileAssert<(bool(**expr))> msg[bool(expr) ? 1 : -1]
> nit: This is kind of a weird mix of static assert techniques to me.
> What's the point of CompileAssert? It seems like you could juse use any
> fundamental type like char and get the same effect in a way that's
> C-compatible:
> typedef char msg[bool(expr) ? 1 : -1];
>
> The kind of canonical C++ way is to do:
> template <bool value>
> struct CompileAssert;
>
> template <>
> struct CompileAssert<true> {};
>
> (I.e., only provide a "true" specialization CompileAssert so that a
> false will cause a compile error that there's no valid specialization
> for "false".)
>
> In the code here, I don't see the purpose of the CompileAssert struct.
>
>
I just blindly copied base/basictypes.h. I'm frankly not sure why it is
this way. Maybe we could learn by studying google3 ;-)

Thanks again!
-Darin


>
https://codereview.chromium.**org/27034003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2013-10-15 17:05:42]

> > mojo/public/bindings/lib/**bindings.h:23: size_t num_bytes =
> > sizeof(Array<T>) + sizeof(StorageType) * num_elements;
> > Do you want to require here that num_bytes is a multiple of 8 bytes? Do
> > you want each element to be on an 8 byte boundary, or is it OK in your
> > design to have 4-byte aligned types land on 4-byte boundaries?
> >
> > I think you're OK, but I wanted to make sure you'd thought about it
> > whether you need to stick an Align() somewhere in the calculation of
> > num_bytes.
> >
> 
> The intent is to allow elements to be packed. It may make sense to insert
> some padding into the array itself. I've been toying with doing that for
> structs as well. Some things would get simpler if each object's size was a
> multiple of 8 bytes. (Objects, arrays and structs, are allocated on 8-byte
> boundaries, but their sizes are not required to be multiples of 8.)
> 
I was thinking that as long as you are respecting the alignment of the contained
type, you'd be fine. But I remembered one of the things we hit in Pepper... 
double is aligned on 8-byte boundaries on some platforms and 4-byte boundaries
on others. So to have a completely platform agnostic format, you might have to
align all elements on 8-byte boundaries :-/. Or have some way of knowing when
it's OK to pack.

This may not be worth worrying about right now, but definitely something to
remember in case you run in to bugs later?

> 
> 
> >
> > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> >
>
public/bindings/lib/bindings.**h#newcode63<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings.h#newcode63>
> > mojo/public/bindings/lib/**bindings.h:63: return
> > reinterpret_cast<StorageType*>**(this + 1);
> > Maybe you've already thought about this, but my understanding was that
> > the result of this conversion is unspecified. (See C++11 standard,
> > 5.2.10p7). I suspect it works in practice right now.
> >
> 
> I see. I suppose I could do some casting then. I was just using this as
> shorthand for:
> reinterpret_cast<StorageType*>(reinterpret_cast<char*>(this) +
> sizeof(*this))
> 
> I also intend to go back and sprinkle in some compile assertions for the
> sizes of
> various types.
The problem is casting the memory to a different type than what it was allocated
as and then de-referencing it. No amount of casting will make it
standards-conforming; you would need to have some kind of StorageType field in
the struct at the right location to make it completely safe cast. HOWEVER...  in
Chromium we do this kind of type-punning enough that we have to build with
-fno-strict-aliasing:
https://code.google.com/p/chromium/issues/detail?id=32204
(you can see also the recent discussion on chromium-dev from the bottom of the
bug).
So...  I think what you have will actually work in practice given our build
flags. It's just not very portable. E.g., this bit me in the NaCl IPC proxy
port, because we currently build the NaCl IRT withOUT -fno-strict-aliasing. We
maybe should add that flag for NaCl.

But...  in general, if we *can* avoid unspecified uses of reinterpret_cast, we
should


> 
> Right. The header needs to be a multiple of 8. For now, I would just assert
> that it is exactly 8.
Okay, thanks, that makes sense.

> 
> 
> 
> >
> > It also seems like doing it this way might obfuscate your code enough
> > that the optimizer won't do as well for at/operator[] as if you simply
> > had the header and an array as members. I think what you might really
> > want is a zero-length array after header_:
> >
>
http://gcc.gnu.org/onlinedocs/**gcc-4.0.4/gcc/Zero-Length.html%3Chttp://gcc.g...>
> > I think that solves a bunch of my above concerns. Alignment should be
> > right, and storage(), at(), and friends can be implemented more
> > naturally.
> >
> 
> I was using something like that before, actually a 1-length array. I wonder
> if zero-length
> is permitted by the standard. I also wonder what it does to the size of an
> object.
Apparently it is not allowed by the standard; sorry about that. It would have to
be a 1-element array, which unfortunately means you either under-allocate memory
for the struct for 0 element Array<> (may be dangerous?), or you use more space
than is really necessary for empty arrays.

:-(

> 
> Again, my plan is to compile assert the size of these structs. The array
> header will
> need to be exactly 8 bytes. Mojo IPC will be prescriptive about such things
> since
> the goal is to invent a binary stable encoding.
Okay. That I think that will work in practice in Chromium's build configuration.
I guess it's up to you whether you want the better portability that you'd get by
adding something like a "StorageType data[1]" (at the cost of extra memory use).
I think I'd lean towards being standards-conforming and having cleaner indexing
code, but I'm not sure how important compactness is.

> 
> 
> >
> > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> >
>
public/bindings/lib/bindings_**serialization.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h>
> > File mojo/public/bindings/lib/**bindings_serialization.h (right):
> >
> > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> >
>
public/bindings/lib/bindings_**serialization.h#newcode123<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h#newcode123>
> > mojo/public/bindings/lib/**bindings_serialization.h:123: }
> > Would it be better to simply leave all of these functions undefined (or
> > omitted)? That way, if somebody tries to specialize ArrayHelper with an
> > unsupported type, they get a compile or link error instead of a runtime
> > error.
> >
> 
> Hmm, I think this template is used for primitive types: e.g.,
> mojo::Array<int32_t>.
Ah. I was confused, expecting if it was the "real" implementation,
ComputeAlignedSizeOfElements it would compute
sizeof(ElementType)*header->num_elements).

I see now that the outer ObjectTraits uses num_bytes which already accounts for
the size of the elements. So I guess my comment is...  I found it a little
confusing, and maybe different names would help?

> I think you meant 12 instead of 24, and yeah... good catch!
Oops, yes, I meant 4*3, which is 12 :-)

> >
> > In the code here, I don't see the purpose of the CompileAssert struct.
> >
> >
> I just blindly copied base/basictypes.h. I'm frankly not sure why it is
> this way. Maybe we could learn by studying google3 ;-)
Ah yes...  apparently it's because GCC supports variable-length arrays whose
sizes are determined at run-time. Passing the bool in to a bool template
paramater just makes sure that it will fail to compile if you give it an
expression that is not statically evaluable:
int foo;
COMPILE_ASSERT(foo, msg); // should fail to compile

Makes sense. Though if you decide you want a C-compatible version (since you do
have some C code), you could drop that part and be careful to only use
statically-evaluable expressions. And...  it will still catch you on MSVC
anyway.

[darin (slow to review), 2013-10-20 22:43:01]

On 2013/10/15 17:05:42, dmichael wrote:
> > > mojo/public/bindings/lib/**bindings.h:23: size_t num_bytes =
> > > sizeof(Array<T>) + sizeof(StorageType) * num_elements;
> > > Do you want to require here that num_bytes is a multiple of 8 bytes? Do
> > > you want each element to be on an 8 byte boundary, or is it OK in your
> > > design to have 4-byte aligned types land on 4-byte boundaries?
> > >
> > > I think you're OK, but I wanted to make sure you'd thought about it
> > > whether you need to stick an Align() somewhere in the calculation of
> > > num_bytes.
> > >
> > 
> > The intent is to allow elements to be packed. It may make sense to insert
> > some padding into the array itself. I've been toying with doing that for
> > structs as well. Some things would get simpler if each object's size was a
> > multiple of 8 bytes. (Objects, arrays and structs, are allocated on 8-byte
> > boundaries, but their sizes are not required to be multiples of 8.)
> > 
> I was thinking that as long as you are respecting the alignment of the
contained
> type, you'd be fine. But I remembered one of the things we hit in Pepper... 
> double is aligned on 8-byte boundaries on some platforms and 4-byte boundaries
> on others. So to have a completely platform agnostic format, you might have to
> align all elements on 8-byte boundaries :-/. Or have some way of knowing when
> it's OK to pack.

It seems like we can insert padding bytes to ensure that members go where we
expect them to go, and we can perhaps also add some compile-time asserts using
offsetof.


> This may not be worth worrying about right now, but definitely something to
> remember in case you run in to bugs later?
> 
> > 
> > 
> > >
> > > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> > >
> >
>
public/bindings/lib/bindings.**h#newcode63<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings.h#newcode63>
> > > mojo/public/bindings/lib/**bindings.h:63: return
> > > reinterpret_cast<StorageType*>**(this + 1);
> > > Maybe you've already thought about this, but my understanding was that
> > > the result of this conversion is unspecified. (See C++11 standard,
> > > 5.2.10p7). I suspect it works in practice right now.
> > >
> > 
> > I see. I suppose I could do some casting then. I was just using this as
> > shorthand for:
> > reinterpret_cast<StorageType*>(reinterpret_cast<char*>(this) +
> > sizeof(*this))
> > 
> > I also intend to go back and sprinkle in some compile assertions for the
> > sizes of
> > various types.
> The problem is casting the memory to a different type than what it was
allocated
> as and then de-referencing it. No amount of casting will make it
> standards-conforming; you would need to have some kind of StorageType field in
> the struct at the right location to make it completely safe cast. HOWEVER... 
in
> Chromium we do this kind of type-punning enough that we have to build with
> -fno-strict-aliasing:
> https://code.google.com/p/chromium/issues/detail?id=32204
> (you can see also the recent discussion on chromium-dev from the bottom of the
> bug).
> So...  I think what you have will actually work in practice given our build
> flags. It's just not very portable. E.g., this bit me in the NaCl IPC proxy
> port, because we currently build the NaCl IRT withOUT -fno-strict-aliasing. We
> maybe should add that flag for NaCl.
> 
> But...  in general, if we *can* avoid unspecified uses of reinterpret_cast, we
> should
>

I was under the impression that casting to a char* helped the compiler
understand
that the address was being aliased, even when strict aliasing is enabled.

 
> 
> > 
> > Right. The header needs to be a multiple of 8. For now, I would just assert
> > that it is exactly 8.
> Okay, thanks, that makes sense.
> 
> > 
> > 
> > 
> > >
> > > It also seems like doing it this way might obfuscate your code enough
> > > that the optimizer won't do as well for at/operator[] as if you simply
> > > had the header and an array as members. I think what you might really
> > > want is a zero-length array after header_:
> > >
> >
>
http://gcc.gnu.org/onlinedocs/**gcc-4.0.4/gcc/Zero-Length.html%253Chttp://gcc...>
> > > I think that solves a bunch of my above concerns. Alignment should be
> > > right, and storage(), at(), and friends can be implemented more
> > > naturally.
> > >
> > 
> > I was using something like that before, actually a 1-length array. I wonder
> > if zero-length
> > is permitted by the standard. I also wonder what it does to the size of an
> > object.
> Apparently it is not allowed by the standard; sorry about that. It would have
to
> be a 1-element array, which unfortunately means you either under-allocate
memory
> for the struct for 0 element Array<> (may be dangerous?), or you use more
space
> than is really necessary for empty arrays.
> 
> :-(

I originally used a data element of length 1. This made some things nicer and
some things messier. I then went with the code you see here. It seemed overall
a bit cleaner. That said, maybe I could be convinced to go back to what I had.
It would also make debugging nicer to have the data element of length 1 ;-)

 
> > 
> > Again, my plan is to compile assert the size of these structs. The array
> > header will
> > need to be exactly 8 bytes. Mojo IPC will be prescriptive about such things
> > since
> > the goal is to invent a binary stable encoding.
> Okay. That I think that will work in practice in Chromium's build
configuration.
> I guess it's up to you whether you want the better portability that you'd get
by
> adding something like a "StorageType data[1]" (at the cost of extra memory
use).
> I think I'd lean towards being standards-conforming and having cleaner
indexing
> code, but I'm not sure how important compactness is.

Compactness is worth the effort. I can again play tricks with struct packing to
perhaps have our cake and eat it to.

By the way, my take on portability is that we should make it work on all
platforms
we care about and make it assert at compile time on any platforms we haven't
covered
and where our non-conformant behavior might matter.


> 
> > 
> > 
> > >
> > > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> > >
> >
>
public/bindings/lib/bindings_**serialization.h<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h>
> > > File mojo/public/bindings/lib/**bindings_serialization.h (right):
> > >
> > > https://codereview.chromium.**org/27034003/diff/2001/mojo/**
> > >
> >
>
public/bindings/lib/bindings_**serialization.h#newcode123<https://codereview.chromium.org/27034003/diff/2001/mojo/public/bindings/lib/bindings_serialization.h#newcode123>
> > > mojo/public/bindings/lib/**bindings_serialization.h:123: }
> > > Would it be better to simply leave all of these functions undefined (or
> > > omitted)? That way, if somebody tries to specialize ArrayHelper with an
> > > unsupported type, they get a compile or link error instead of a runtime
> > > error.
> > >
> > 
> > Hmm, I think this template is used for primitive types: e.g.,
> > mojo::Array<int32_t>.
> Ah. I was confused, expecting if it was the "real" implementation,
> ComputeAlignedSizeOfElements it would compute
> sizeof(ElementType)*header->num_elements).
> 
> I see now that the outer ObjectTraits uses num_bytes which already accounts
for
> the size of the elements. So I guess my comment is...  I found it a little
> confusing, and maybe different names would help?

I need to think about it. I'm also toying with the idea of allocating
array elements, which correspond to objects, inline rather than as pointers.
That would shake things up a bit w.r.t. this code.


> 
> > I think you meant 12 instead of 24, and yeah... good catch!
> Oops, yes, I meant 4*3, which is 12 :-)
> 
> > >
> > > In the code here, I don't see the purpose of the CompileAssert struct.
> > >
> > >
> > I just blindly copied base/basictypes.h. I'm frankly not sure why it is
> > this way. Maybe we could learn by studying google3 ;-)
> Ah yes...  apparently it's because GCC supports variable-length arrays whose
> sizes are determined at run-time. Passing the bool in to a bool template
> paramater just makes sure that it will fail to compile if you give it an
> expression that is not statically evaluable:
> int foo;
> COMPILE_ASSERT(foo, msg); // should fail to compile
> 
> Makes sense. Though if you decide you want a C-compatible version (since you
do
> have some C code), you could drop that part and be careful to only use
> statically-evaluable expressions. And...  it will still catch you on MSVC
> anyway.

Ah, thanks for digging that up. I had always wondered.

-Darin