C++ bindings

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.

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

[yzshen1, 2013-09-24 18:32:56]

(Sorry if the question is naive.)

https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test.cc
File mojo/public/bindings/test.cc (right):

https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test...
mojo/public/bindings/test.cc:77: *offset = p_obj - p_slot;
Is it intended to use an unsigned number for |offset|?
The pointer subtraction could result in negative values. Or do we want to layout
messages in a way that guarantees |offset| is always positive?

Do we assume that real pointers (outside of messages) are always 64-bit?

[darin (slow to review), 2013-09-24 18:45:51]

Those are great questions. I did have the idea that offsets should be
positive, but they don't really need to be. They could be signed. I was
going to think about this aspect when I go to write validation code. It
would be useful to validate that offsets are not duplicated, do not
reference overlapping regions of memory, and do not introduce cycles.

Real pointers just have to fit within 64-bits. That should be reasonable
for a long time to come.

-Darin


On Tue, Sep 24, 2013 at 11:32 AM, <yzshen@chromium.org> wrote:

> (Sorry if the question is naive.)
>
>
> https://codereview.chromium.**org/23913008/diff/25001/mojo/**
>
public/bindings/test.cc<https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test.cc>
> File mojo/public/bindings/test.cc (right):
>
> https://codereview.chromium.**org/23913008/diff/25001/mojo/**
>
public/bindings/test.cc#**newcode77<https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test.cc#newcode77>
> mojo/public/bindings/test.cc:**77: *offset = p_obj - p_slot;
> Is it intended to use an unsigned number for |offset|?
> The pointer subtraction could result in negative values. Or do we want
> to layout messages in a way that guarantees |offset| is always positive?
>
> Do we assume that real pointers (outside of messages) are always 64-bit?
>
>
https://codereview.chromium.**org/23913008/<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.

[yzshen, 2013-09-24 18:53:53]

Thanks, Darin!

On Tue, Sep 24, 2013 at 11:45 AM, Darin Fisher <darin@chromium.org> wrote:

> Those are great questions. I did have the idea that offsets should be
> positive, but they don't really need to be. They could be signed. I was
> going to think about this aspect when I go to write validation code. It
> would be useful to validate that offsets are not duplicated, do not
> reference overlapping regions of memory, and do not introduce cycles.
>

Right, and also don't refer to somewhere beyond the message boundary.


>
> Real pointers just have to fit within 64-bits. That should be reasonable
> for a long time to come.
>
> -Darin
>
>
> On Tue, Sep 24, 2013 at 11:32 AM, <yzshen@chromium.org> wrote:
>
>> (Sorry if the question is naive.)
>>
>>
>> https://codereview.chromium.**org/23913008/diff/25001/mojo/**
>>
public/bindings/test.cc<https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test.cc>
>> File mojo/public/bindings/test.cc (right):
>>
>> https://codereview.chromium.**org/23913008/diff/25001/mojo/**
>>
public/bindings/test.cc#**newcode77<https://codereview.chromium.org/23913008/diff/25001/mojo/public/bindings/test.cc#newcode77>
>> mojo/public/bindings/test.cc:**77: *offset = p_obj - p_slot;
>> Is it intended to use an unsigned number for |offset|?
>> The pointer subtraction could result in negative values. Or do we want
>> to layout messages in a way that guarantees |offset| is always positive?
>>
>> Do we assume that real pointers (outside of messages) are always 64-bit?
>>
>>
https://codereview.chromium.**org/23913008/<https://codereview.chromium.org/2...
>>
>
>


-- 
Best regards,
Yuzhu Shen.

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

[piman, 2013-09-25 17:26:24]

https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test.cc
File mojo/public/bindings/test.cc (right):

https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test...
mojo/public/bindings/test.cc:96: *ptr = reinterpret_cast<T*>(p_slot + *offset);
Note: strict aliasing rules only allow reinterpret_cast from char* not unsigned
char*. I think we want to keep the data_start/end in Message as char*.

[Hajime Morrita, 2013-09-25 20:03:55]

https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test.cc
File mojo/public/bindings/test.cc (right):

https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test...
mojo/public/bindings/test.cc:296: StructHeader header_;
One possible alternative is to keep the message class a plain C++ style
and give the header and related methods using some decorator-like class:

class Serializable<T> {
  StructHeader header_;
  T data_;
  ... reading/writing from the buffer.
};

class Foo {
  uint8_t _alpha;
  uint8_t _beta;
  uint8_t _gamma;

  ... setters and getters, etc. follow ...

  operator Serializable<Foo>() { ... };
};

typedef Serializable<Foo> SerializableFoo;

---

By keeping serialization overhead out from the generated class,
we can use the classes in applications without worry about the overhead.
Otherwise, people may create "native" classes 
which are converted to message classes on sending.
This especially helps when we use arrays of these message classes.

[darin (slow to review), 2013-09-28 04:53:06]

On 2013/09/25 17:26:24, piman wrote:
>
https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test.cc
> File mojo/public/bindings/test.cc (right):
> 
>
https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test...
> mojo/public/bindings/test.cc:96: *ptr = reinterpret_cast<T*>(p_slot +
*offset);
> Note: strict aliasing rules only allow reinterpret_cast from char* not
unsigned
> char*. I think we want to keep the data_start/end in Message as char*.

Thanks. I didn't realize there was a different between unsigned and signed char
regarding strict aliasing rules.

[darin (slow to review), 2013-09-28 05:03:54]

On 2013/09/25 20:03:55, morrita1 wrote:
>
https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test.cc
> File mojo/public/bindings/test.cc (right):
> 
>
https://codereview.chromium.org/23913008/diff/31001/mojo/public/bindings/test...
> mojo/public/bindings/test.cc:296: StructHeader header_;
> One possible alternative is to keep the message class a plain C++ style
> and give the header and related methods using some decorator-like class:
> 
> class Serializable<T> {
>   StructHeader header_;
>   T data_;
>   ... reading/writing from the buffer.
> };
> 
> class Foo {
>   uint8_t _alpha;
>   uint8_t _beta;
>   uint8_t _gamma;
> 
>   ... setters and getters, etc. follow ...
> 
>   operator Serializable<Foo>() { ... };
> };
> 
> typedef Serializable<Foo> SerializableFoo;
> 
> ---
> 
> By keeping serialization overhead out from the generated class,
> we can use the classes in applications without worry about the overhead.
> Otherwise, people may create "native" classes 
> which are converted to message classes on sending.
> This especially helps when we use arrays of these message classes.

Thanks for the suggestion. I had gone down a path like this as well,
but some concerns arise:

1) Serializable<Foo> needs to know about various members of Foo that
are pointers to other objects and arrays. It needs this so it can
traverse the tree when cloning the tree or encoding/decoding pointers
and handles.

2) I'd like to be able to just reinterpret_cast the incoming message
buffer to a Foo*. It is possible for the message to be truncated.
The StructHeader indicates how many fields are present. This means
some field accessors will need to check how many fields are present
before the memory can be dereferenced. So, instead of a struct, I
end up needing a class with access to the field count. The field
count is stored in the StructHeader.

An alternative approach to dealing with the issue raised in #2 would
be to copy the struct out of the incoming message buffer into a
buffer that is large enough for the size of the entire struct. Then
just zero-fill the portion that was not present in the incoming
message buffer. That would work, but it would increase cost to
support those memcpy operations.

Stepping back, I'm also not entirely sure that people would be best
served by using these structs for more than just marshalling calls
to services. Because strings and arrays are not represented normally
(i.e., as std::string and std::vector<T>), these structs really aren't
ideal for storing and using in client code. It seems like folks will
generally want to use their own data structures when they wish to
copy some data received over Mojo IPC.

I'm not as confident in the last point. It is possible that for simpler
structs (those without embedded pointers) that people would be quite
happy re-using these structs in other parts of their code. I'd like to
try converting some services over to Mojo IPC to see how it feels before
committing one way or the other.

[darin (slow to review), 2013-09-28 07:03:25]

In the latest CL, everything that isn't needed by clients is moved out of
sample_service.h into sample_service_internal.h. The Clone and
{Encode,Decode}PointersAndHandles methods now live on Traits<T> classes defined
in sample_service_internal.h.

This should help make the generated classes much more friendly to the eyes.

[Ben Goodger (Google), 2013-09-28 21:15:29]

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/gene...
File mojo/public/bindings/generated/sample_service.h (right):

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/gene...
mojo/public/bindings/generated/sample_service.h:10: namespace sample {
this looks much better

[viettrungluu, 2013-10-01 00:51:03]

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/gene...
File mojo/public/bindings/generated/sample_service.h (right):

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/gene...
mojo/public/bindings/generated/sample_service.h:31: struct {
This is nice for debugging, but apart from that are there any advantages to this
over just having char data[...]; and writing the getters and setters using
reinterpret_cast?

(Also, for debugging purposes, it might be nice to give the struct a name.)

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/gene...
mojo/public/bindings/generated/sample_service.h:63: const Bar* bar() const {
return d_.bar.ptr; }
Would there be value in also have a (non-const) |Bar* mutable_bar();| method
(and similarly for |data| and |extra_bars|).

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/samp...
File mojo/public/bindings/sample_service.idl (right):

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/samp...
mojo/public/bindings/sample_service.idl:13: };
Are we going to support ...

- ... "enums"? Enumerations that get checked properly are probably a
usability/security win.
- ... inner structs? (Side comment: protoc declares an inner struct/message
Foo::Bar as Foo_Bar + a typedef to allow Foo::Bar to be used, which is nice
since it allows Foo_Bar to be forward-declared.
- ... inner (nested) namespaces?
- ... importing other IDL files? (Side comment: The latest protobufs
distinguishes between "import" and "import public", which is nice.
Forward-declarability is required for this.)

(Protobufs support all of these. Probably some of my questions have been
answered somewhere, but I'm too lazy to dig through design docs at the moment.)

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/samp...
File mojo/public/bindings/sample_test.cc (right):

https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/samp...
mojo/public/bindings/sample_test.cc:154: foo->set_bar(bar);
I'm a little concerned that this and (even more so) set_extra_bars() may be too
inconvenient and error-prone to use. (Though I guess you did say we'll probably
end up with more sugar.)

[darin (slow to review), 2013-10-01 03:50:17]

On Mon, Sep 30, 2013 at 5:51 PM, <viettrungluu@chromium.org> wrote:

>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/generated/**sample_service.h<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h>
> File mojo/public/bindings/**generated/sample_service.h (right):
>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/generated/**sample_service.h#newcode31<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode31>
> mojo/public/bindings/**generated/sample_service.h:31: struct {
> This is nice for debugging, but apart from that are there any advantages
> to this over just having char data[...]; and writing the getters and
> setters using reinterpret_cast?
>
>
I don't know. Making this friendly to debuggers was my main motivation.



> (Also, for debugging purposes, it might be nice to give the struct a
> name.)
>

Hmm... do you think it needs to be a non-nested name? How does naming it
help with debugging? Are you saying so you can do things like "p
(Foo_Data*) address" in gdb?



>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/generated/**sample_service.h#newcode63<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode63>
> mojo/public/bindings/**generated/sample_service.h:63: const Bar* bar()
> const { return d_.bar.ptr; }
> Would there be value in also have a (non-const) |Bar* mutable_bar();|
> method (and similarly for |data| and |extra_bars|).
>

Perhaps. It could be useful if you wanted to avoid a temporary at the
construction site:

foo->set_bar(... alloc Bar...);
foo->bar()->set_alpha(...);
foo->bar()->set_beta(...);
foo->bar()->set_gamma(...);

I'm not sure this is worth it if the cost is having to type the "mutable_"
prefix though. Do you think this would be useful?



>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/sample_**service.idl<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl>
> File mojo/public/bindings/sample_**service.idl (right):
>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/sample_**service.idl#newcode13<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl#newcode13>
> mojo/public/bindings/sample_**service.idl:13: };
> Are we going to support ...
>
> - ... "enums"? Enumerations that get checked properly are probably a
> usability/security win.
> - ... inner structs? (Side comment: protoc declares an inner
> struct/message Foo::Bar as Foo_Bar + a typedef to allow Foo::Bar to be
> used, which is nice since it allows Foo_Bar to be forward-declared.
> - ... inner (nested) namespaces?
> - ... importing other IDL files? (Side comment: The latest protobufs
> distinguishes between "import" and "import public", which is nice.
> Forward-declarability is required for this.)
>

I have left these off for now. I think they can be supported. I'd probably
prioritize enums over the others.



>
> (Protobufs support all of these. Probably some of my questions have been
> answered somewhere, but I'm too lazy to dig through design docs at the
> moment.)
>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/sample_test.cc<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc>
> File mojo/public/bindings/sample_**test.cc (right):
>
> https://codereview.chromium.**org/23913008/diff/44001/mojo/**
>
public/bindings/sample_test.**cc#newcode154<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc#newcode154>
> mojo/public/bindings/sample_**test.cc:154: foo->set_bar(bar);
> I'm a little concerned that this and (even more so) set_extra_bars() may
> be too inconvenient and error-prone to use. (Though I guess you did say
> we'll probably end up with more sugar.)
>

Right. Tell me more about why you think the set_bar(...) method is too
error prone. Are you referring to the way in which Bar gets allocated? For
arrays, I was planning on adding convenient methods for generating
mojo::Array<T> from std::vector<T>. Ditto for std::string.

-Darin



>
>
https://codereview.chromium.**org/23913008/<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.

[viettrungluu, 2013-10-02 00:52:47]

On 2013/10/01 03:50:17, darin wrote:
> On Mon, Sep 30, 2013 at 5:51 PM, <mailto:viettrungluu@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/generated/**sample_service.h<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h>
> > File mojo/public/bindings/**generated/sample_service.h (right):
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/generated/**sample_service.h#newcode31<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode31>
> > mojo/public/bindings/**generated/sample_service.h:31: struct {
> > This is nice for debugging, but apart from that are there any advantages
> > to this over just having char data[...]; and writing the getters and
> > setters using reinterpret_cast?
> >
> >
> I don't know. Making this friendly to debuggers was my main motivation.
> 
> 
> 
> > (Also, for debugging purposes, it might be nice to give the struct a
> > name.)
> >
> 
> Hmm... do you think it needs to be a non-nested name? How does naming it
> help with debugging? Are you saying so you can do things like "p
> (Foo_Data*) address" in gdb?

Yes, I mean exactly that sort of thing in gdb. It shouldn't need to be a
non-nested name. (Non-nested names are mostly useful for non-promiscuous
imports, since you can forward-declare them.)

> 
> 
> 
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/generated/**sample_service.h#newcode63<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode63>
> > mojo/public/bindings/**generated/sample_service.h:63: const Bar* bar()
> > const { return d_.bar.ptr; }
> > Would there be value in also have a (non-const) |Bar* mutable_bar();|
> > method (and similarly for |data| and |extra_bars|).
> >
> 
> Perhaps. It could be useful if you wanted to avoid a temporary at the
> construction site:
> 
> foo->set_bar(... alloc Bar...);
> foo->bar()->set_alpha(...);
> foo->bar()->set_beta(...);
> foo->bar()->set_gamma(...);
> 
> I'm not sure this is worth it if the cost is having to type the "mutable_"
> prefix though. Do you think this would be useful?

I was mostly thinking of the case when things are bigger, especially if we think
that people may want to use our structs for more than just IPC (e.g., using them
for internal storage, serialized external storage, etc., like protobufs are).

> 
> 
> 
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/sample_**service.idl<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl>
> > File mojo/public/bindings/sample_**service.idl (right):
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/sample_**service.idl#newcode13<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl#newcode13>
> > mojo/public/bindings/sample_**service.idl:13: };
> > Are we going to support ...
> >
> > - ... "enums"? Enumerations that get checked properly are probably a
> > usability/security win.
> > - ... inner structs? (Side comment: protoc declares an inner
> > struct/message Foo::Bar as Foo_Bar + a typedef to allow Foo::Bar to be
> > used, which is nice since it allows Foo_Bar to be forward-declared.
> > - ... inner (nested) namespaces?
> > - ... importing other IDL files? (Side comment: The latest protobufs
> > distinguishes between "import" and "import public", which is nice.
> > Forward-declarability is required for this.)
> >
> 
> I have left these off for now. I think they can be supported. I'd probably
> prioritize enums over the others.
> 
> 
> 
> >
> > (Protobufs support all of these. Probably some of my questions have been
> > answered somewhere, but I'm too lazy to dig through design docs at the
> > moment.)
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/sample_test.cc<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc>
> > File mojo/public/bindings/sample_**test.cc (right):
> >
> > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> >
>
public/bindings/sample_test.**cc#newcode154<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc#newcode154>
> > mojo/public/bindings/sample_**test.cc:154: foo->set_bar(bar);
> > I'm a little concerned that this and (even more so) set_extra_bars() may
> > be too inconvenient and error-prone to use. (Though I guess you did say
> > we'll probably end up with more sugar.)
> >
> 
> Right. Tell me more about why you think the set_bar(...) method is too
> error prone. Are you referring to the way in which Bar gets allocated? For
> arrays, I was planning on adding convenient methods for generating
> mojo::Array<T> from std::vector<T>. Ditto for std::string.

Yes, I was referring to the need to allocate stuff. I'm going to stare at the
"generated" code some more....

> 
> -Darin
> 
> 
> 
> >
> >
>
https://codereview.chromium.**org/23913008/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2013-10-02 01:17:06]

https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
File mojo/public/bindings/generated/sample_service_proxy.cc (right):

https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
mojo/public/bindings/generated/sample_service_proxy.cc:27:
internal::Service_Frobinate_Params* params =
I wonder if in the case that params is small (and has no arrays/pointers), we
should allocate this on the stack.

https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
mojo/public/bindings/generated/sample_service_proxy.cc:45: message.data_start =
buf.data();
... which makes me think that MessageBuffer should reserve some space for things
like this.

It'd be nice if Send() could just take a fully serialized message (including the
name) + a separate handles array, so that it could pass these off immediately to
the low-level C primitive. (Or, we could add the message type as an explicit
field to the low-level SendMessage(). This is okay, since a copy is necessary
between "user" and "kernel" space anyway.)

[darin (slow to review), 2013-10-02 04:49:51]

On 2013/10/02 01:17:06, viettrungluu wrote:
>
https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
> File mojo/public/bindings/generated/sample_service_proxy.cc (right):
> 
>
https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
> mojo/public/bindings/generated/sample_service_proxy.cc:27:
> internal::Service_Frobinate_Params* params =
> I wonder if in the case that params is small (and has no arrays/pointers), we
> should allocate this on the stack.

It is allocated as part of the message buffer so that we can do what you suggest
below...


>
https://codereview.chromium.org/23913008/diff/62001/mojo/public/bindings/gene...
> mojo/public/bindings/generated/sample_service_proxy.cc:45: message.data_start
=
> buf.data();
> ... which makes me think that MessageBuffer should reserve some space for
things
> like this.
> 
> It'd be nice if Send() could just take a fully serialized message (including
the
> name) + a separate handles array, so that it could pass these off immediately
to
> the low-level C primitive. (Or, we could add the message type as an explicit
> field to the low-level SendMessage(). This is okay, since a copy is necessary
> between "user" and "kernel" space anyway.)

Yes, I have a TODO about this somewhere.

[darin (slow to review), 2013-10-02 04:55:42]

On 2013/10/02 00:52:47, viettrungluu wrote:
> On 2013/10/01 03:50:17, darin wrote:
> > On Mon, Sep 30, 2013 at 5:51 PM, <mailto:viettrungluu@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/generated/**sample_service.h<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h>
> > > File mojo/public/bindings/**generated/sample_service.h (right):
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/generated/**sample_service.h#newcode31<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode31>
> > > mojo/public/bindings/**generated/sample_service.h:31: struct {
> > > This is nice for debugging, but apart from that are there any advantages
> > > to this over just having char data[...]; and writing the getters and
> > > setters using reinterpret_cast?
> > >
> > >
> > I don't know. Making this friendly to debuggers was my main motivation.
> > 
> > 
> > 
> > > (Also, for debugging purposes, it might be nice to give the struct a
> > > name.)
> > >
> > 
> > Hmm... do you think it needs to be a non-nested name? How does naming it
> > help with debugging? Are you saying so you can do things like "p
> > (Foo_Data*) address" in gdb?
> 
> Yes, I mean exactly that sort of thing in gdb. It shouldn't need to be a
> non-nested name. (Non-nested names are mostly useful for non-promiscuous
> imports, since you can forward-declare them.)

OK, I will go with the generic name of "Data" to match the "d_" variable.

> 
> > 
> > 
> > 
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/generated/**sample_service.h#newcode63<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/generated/sample_service.h#newcode63>
> > > mojo/public/bindings/**generated/sample_service.h:63: const Bar* bar()
> > > const { return d_.bar.ptr; }
> > > Would there be value in also have a (non-const) |Bar* mutable_bar();|
> > > method (and similarly for |data| and |extra_bars|).
> > >
> > 
> > Perhaps. It could be useful if you wanted to avoid a temporary at the
> > construction site:
> > 
> > foo->set_bar(... alloc Bar...);
> > foo->bar()->set_alpha(...);
> > foo->bar()->set_beta(...);
> > foo->bar()->set_gamma(...);
> > 
> > I'm not sure this is worth it if the cost is having to type the "mutable_"
> > prefix though. Do you think this would be useful?
> 
> I was mostly thinking of the case when things are bigger, especially if we
think
> that people may want to use our structs for more than just IPC (e.g., using
them
> for internal storage, serialized external storage, etc., like protobufs are).

Hmm, are you referring to the use case of updating only a couple of fields in
an already constructed message? I'm not sure this format is so ideal for that
kind of use case. It works better for serialize, store, de-serialize use cases.
That said, we could certainly optimize the bindings for such use cases later.
I'm tempted to not worry about such use cases now.


> 
> > 
> > 
> > 
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/sample_**service.idl<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl>
> > > File mojo/public/bindings/sample_**service.idl (right):
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/sample_**service.idl#newcode13<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_service.idl#newcode13>
> > > mojo/public/bindings/sample_**service.idl:13: };
> > > Are we going to support ...
> > >
> > > - ... "enums"? Enumerations that get checked properly are probably a
> > > usability/security win.
> > > - ... inner structs? (Side comment: protoc declares an inner
> > > struct/message Foo::Bar as Foo_Bar + a typedef to allow Foo::Bar to be
> > > used, which is nice since it allows Foo_Bar to be forward-declared.
> > > - ... inner (nested) namespaces?
> > > - ... importing other IDL files? (Side comment: The latest protobufs
> > > distinguishes between "import" and "import public", which is nice.
> > > Forward-declarability is required for this.)
> > >
> > 
> > I have left these off for now. I think they can be supported. I'd probably
> > prioritize enums over the others.
> > 
> > 
> > 
> > >
> > > (Protobufs support all of these. Probably some of my questions have been
> > > answered somewhere, but I'm too lazy to dig through design docs at the
> > > moment.)
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/sample_test.cc<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc>
> > > File mojo/public/bindings/sample_**test.cc (right):
> > >
> > > https://codereview.chromium.**org/23913008/diff/44001/mojo/**
> > >
> >
>
public/bindings/sample_test.**cc#newcode154<https://codereview.chromium.org/23913008/diff/44001/mojo/public/bindings/sample_test.cc#newcode154>
> > > mojo/public/bindings/sample_**test.cc:154: foo->set_bar(bar);
> > > I'm a little concerned that this and (even more so) set_extra_bars() may
> > > be too inconvenient and error-prone to use. (Though I guess you did say
> > > we'll probably end up with more sugar.)
> > >
> > 
> > Right. Tell me more about why you think the set_bar(...) method is too
> > error prone. Are you referring to the way in which Bar gets allocated? For
> > arrays, I was planning on adding convenient methods for generating
> > mojo::Array<T> from std::vector<T>. Ditto for std::string.
> 
> Yes, I was referring to the need to allocate stuff. I'm going to stare at the
> "generated" code some more....
>

Right. I think I can make that prettier. I think I can get away with stack
based allocations of structs. The key is finding a good way to deal with
arrays, which need to be heap allocated. I could just let people allocate
them on the heap and manage those pointers using scoped_ptr<T> as usual.
The one thing I need to do is disallow copy-constructors and assignment
operators, but I think that should be quite acceptable. (Upon receiving a
message, I don't want people copying the objects or arrays as that would
create challenges with memory management.)

 
> > 
> > -Darin
> > 
> > 
> > 
> > >
> > >
> >
>
https://codereview.chromium.**org/23913008/%253Chttps://codereview.chromium.o...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-10-09 00:05:19]

trung: PTAL

[viettrungluu, 2013-10-09 03:44:06]

Mostly it looks good (apart from some nitpicky const-correctness, etc. issues),
though I want to read it again tomorrow.

https://codereview.chromium.org/23913008/diff/110001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/23913008/diff/110001/mojo/mojo.gyp#newcode72
mojo/mojo.gyp:72: 'public/system/core.h',
Hmmm, I wonder if this should have been listed here (in which case it and
macros.h should also be listed elsewhere too).

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings.h (right):

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings.h:85: (num_elements - 1);
Can num_elements be 0?

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings_internal.cc (right):

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.cc:12: void EncodePointer(void*
address, uint64_t* offset) {
const void* address?

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.cc:17: uint8_t* p_obj =
reinterpret_cast<uint8_t*>(address);
static_cast

Though maybe you should be using uintptr_t (and reinterpret_cast'ing).

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.cc:20: *offset = p_obj - p_slot;
(And there should be a cast here too, I think.)

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.cc:23: uint8_t*
DecodePointerRaw(uint64_t* offset) {
Probably this should return void*? (And take a const uint64_t*? This is more
debatable.)

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.h:18: void EncodePointer(void*
address, uint64_t* offset);
Some more comments would be nice. :)

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.h:19: uint8_t*
DecodePointerRaw(uint64_t* offset);
(I think Antoine mentioned this.) It'd be more C++-correct/safer to have it
return a char or unsigned char (since aliasing rules call out the char types
specifically; e.g., theoretically, Microsoft could assume that __uint8 pointers
don't alias).

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.cc (right):

https://codereview.chromium.org/23913008/diff/110001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.cc:14: namespace {
(Note that the anonymous namespace is redundant, since consts have internal
linkage by default in C++.)

[viettrungluu, 2013-10-09 17:00:24]

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.cc (right):

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.cc:106: size_ = 0;
Also, |capacity_ = 0;|?

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.h (right):

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:30: static const size_t kMinSegmentSize = 512;
Note that C++ requires that you declare storage for static const members. Except
that MSVS doesn't handle this correctly (so for MSVS you shouldn't).

(Not having storage will often lead to link errors if you ever need to pass the
constant as a reference argument, e.g., to various STL functions.)

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:46: StackBuffer(const StackBuffer&);
I wonder if we should have a MOJO_DISALLOW_COPY_AND_ASSIGN()....

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:65: //     double* b =
static_cast<double*>(buf.Grow(sizeof(double)));
There's no |Grow()| anymore....

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:82: virtual void* Allocate(size_t num_bytes)
MOJO_OVERRIDE;
So it seems problematic that it might invalidate previously-returned pointers
(if it reallocs) ... which means that you can only use a pointer returned by
Allocate() until the next call to Allocate().

[darin (slow to review), 2013-10-09 17:05:31]

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.cc (right):

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.cc:106: size_ = 0;
On 2013/10/09 17:00:24, viettrungluu wrote:
> Also, |capacity_ = 0;|?

Yes, good catch.

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.h (right):

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:22: class StackBuffer : public Buffer {
I was thinking this class might be better named ScratchBuffer. It is not just a
buffer for allocating objects on the stack as it will failover to the heap if
necessary. From the perspective of users, this is the buffer that will almost
always be used. Maybe it should be called ScopedBuffer to make it clear that it
sets up a scope under which objects may be temporarily allocated. Thoughts?

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:30: static const size_t kMinSegmentSize = 512;
On 2013/10/09 17:00:24, viettrungluu wrote:
> Note that C++ requires that you declare storage for static const members.
Except
> that MSVS doesn't handle this correctly (so for MSVS you shouldn't).
> 
> (Not having storage will often lead to link errors if you ever need to pass
the
> constant as a reference argument, e.g., to various STL functions.)

OK

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:46: StackBuffer(const StackBuffer&);
On 2013/10/09 17:00:24, viettrungluu wrote:
> I wonder if we should have a MOJO_DISALLOW_COPY_AND_ASSIGN()....

Yes, I wanted to introduce that. Now that we have macros.h, that'll be easy to
do.

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:65: //     double* b =
static_cast<double*>(buf.Grow(sizeof(double)));
On 2013/10/09 17:00:24, viettrungluu wrote:
> There's no |Grow()| anymore....

Oops, yes. I renamed it.

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:82: virtual void* Allocate(size_t num_bytes)
MOJO_OVERRIDE;
On 2013/10/09 17:00:24, viettrungluu wrote:
> So it seems problematic that it might invalidate previously-returned pointers
> (if it reallocs) ... which means that you can only use a pointer returned by
> Allocate() until the next call to Allocate().

Oh, good catch. I need to think about that.

[DaveMoore, 2013-10-09 17:14:06]

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings.h (right):

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings.h:7: 
I find the naming of this header confusing, at least as it stands now.

To an external consumer it defines one public type, mojo::Array<T>, but it's
called "bindings.h".

For an internal consumer it defines all the rest of the types.

I think those internal types should at a minimum be broken out into their own
header that clearly indicates that they're for internal consumption only, and
have a public header that's specific to Array<T>. But we already have
bindings_internal.h. That seems odd to me.

In general there's an awful lot of magic here and I'm concerned that public
consumers of the real apis are going to have to understand a lot of it.

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.h:22: //   ptr = (offset + *offset)
So these are array style offsets (numbytes / 8)? A little surprising, but I
guess it's ok.

From the code it looks like offset is supposed to be numbytes (as you cast), so
either the comment or code should be changed.

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_internal.h:51: inline T* Clone(const T* obj,
Buffer* buf) {
It might be better to make this an error (or not check). Is cloning NULL really
valid?

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.h (right):

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:65: //     double* b =
static_cast<double*>(buf.Grow(sizeof(double)));
Where is this Grow() method you speak of?

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/sam...
File mojo/public/bindings/sample/generated/sample_service.h (right):

https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/sam...
mojo/public/bindings/sample/generated/sample_service.h:106:
mojo::internal::ArrayPointer<mojo::Handle> files;
This is pretty fugly. I'm still not convinced that it's better to push all the
data fields into a nested structure instead of reserving some safe name for the
header. And all the mojo::internals make this pretty hard to read.

[viettrungluu, 2013-10-09 20:54:26]

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/buffer.h (right):

https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib...
mojo/public/bindings/lib/buffer.h:22: class StackBuffer : public Buffer {
On 2013/10/09 17:05:32, darin wrote:
> I was thinking this class might be better named ScratchBuffer. It is not just
a
> buffer for allocating objects on the stack as it will failover to the heap if
> necessary. From the perspective of users, this is the buffer that will almost
> always be used. Maybe it should be called ScopedBuffer to make it clear that
it
> sets up a scope under which objects may be temporarily allocated. Thoughts?

I think I like ScratchBuffer more. (To me, ScopedBuffer suggests that you could
"detach" objects in it for use elsewhere, or detach the buffer.)

[darin (slow to review), 2013-10-09 23:41:37]

On Wed, Oct 9, 2013 at 10:14 AM, <davemoore@chromium.org> wrote:

>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/bindings.h<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/bindings.h>
> File mojo/public/bindings/lib/**bindings.h (right):
>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/bindings.**h#newcode7<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/bindings.h#newcode7>
> mojo/public/bindings/lib/**bindings.h:7:
> I find the naming of this header confusing, at least as it stands now.
>
> To an external consumer it defines one public type, mojo::Array<T>, but
> it's called "bindings.h".
>
> For an internal consumer it defines all the rest of the types.
>
> I think those internal types should at a minimum be broken out into
> their own header that clearly indicates that they're for internal
> consumption only, and have a public header that's specific to Array<T>.
> But we already have bindings_internal.h. That seems odd to me.
>

Yeah, I wrestled with this too. If you are looking for a header to read that
defines public classes you can interact with, then I agree that bindings.h
does not seem ideal. What it really represents however is the set of types
that have to be visible to consumers. OTOH, bindings_internal.h represents
the set of private types that are only needed when compiling the generated
*_proxy.cc and *_stub.cc files.

Both distinctions seem interesting. I had considered renaming
bindings_internal.h
to bindings_impl.h, but I'm not sure this is right either since it doesn't
correspond
to the implementation of something defined in bindings.h. I need to think
about
this more.



>
> In general there's an awful lot of magic here and I'm concerned that
> public consumers of the real apis are going to have to understand a lot
> of it.
>

Are you referring to mojo::Array<T> in particular, or are you talking about
the treatment of structs (e.g., sample::Foo and sample::Bar) as well?



>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/bindings_**internal.h<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/bindings_internal.h>
> File mojo/public/bindings/lib/**bindings_internal.h (right):
>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/bindings_**internal.h#newcode22<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/bindings_internal.h#newcode22>
> mojo/public/bindings/lib/**bindings_internal.h:22: //   ptr = (offset +
> *offset)
> So these are array style offsets (numbytes / 8)? A little surprising,
> but I guess it's ok.
>
> From the code it looks like offset is supposed to be numbytes (as you
> cast), so either the comment or code should be changed.
>

Does changing the comment to the following address your concern?

//   ptr = reinterpret_cast<char*>(offset) + *offset

I just added the reinterpret_cast to make it more precise. By the way,
since offsets are aligned to 8 bytes, it would be reasonable for them
to be multiples of 8, so that you could indeed to pointer math with an
uint64_t*. Do you think that would be better?



>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/bindings_**internal.h#newcode51<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/bindings_internal.h#newcode51>
> mojo/public/bindings/lib/**bindings_internal.h:51: inline T* Clone(const
> T* obj, Buffer* buf) {
> It might be better to make this an error (or not check). Is cloning NULL
> really valid?
>

It is valid for a struct to have a member that is a null-pointer to another
struct. I could
prefix all calls to Clone(...) with a null-check, but the code is a lot
cleaner if we just
do it once here. Take a look at ObjectTraits<sample::Foo>::Clone(...) to
see what I
mean.


>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/buffer.h<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/buffer.h>
> File mojo/public/bindings/lib/**buffer.h (right):
>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/lib/buffer.h#**newcode65<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/lib/buffer.h#newcode65>
>
> mojo/public/bindings/lib/**buffer.h:65: //     double* b =
> static_cast<double*>(buf.Grow(**sizeof(double)));
> Where is this Grow() method you speak of?
>

Typo


>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/sample/**generated/sample_service.h<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/sample/generated/sample_service.h>
> File mojo/public/bindings/sample/**generated/sample_service.h (right):
>
> https://codereview.chromium.**org/23913008/diff/133001/mojo/**
>
public/bindings/sample/**generated/sample_service.h#**newcode106<https://codereview.chromium.org/23913008/diff/133001/mojo/public/bindings/sample/generated/sample_service.h#newcode106>
> mojo/public/bindings/sample/**generated/sample_service.h:**106:
> mojo::internal::ArrayPointer<**mojo::Handle> files;
> This is pretty fugly. I'm still not convinced that it's better to push
> all the data fields into a nested structure instead of reserving some
> safe name for the header. And all the mojo::internals make this pretty
> hard to read.
>

Do you really think it is that important to optimize this particular code
for
readability?

-Darin



>
>
https://codereview.chromium.**org/23913008/<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.

[darin (slow to review), 2013-10-09 23:44:29]

On Wed, Oct 9, 2013 at 1:54 PM, <viettrungluu@chromium.org> wrote:

>
> https://codereview.chromium.**org/23913008/diff/146001/mojo/**
>
public/bindings/lib/buffer.h<https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib/buffer.h>
> File mojo/public/bindings/lib/**buffer.h (right):
>
> https://codereview.chromium.**org/23913008/diff/146001/mojo/**
>
public/bindings/lib/buffer.h#**newcode22<https://codereview.chromium.org/23913008/diff/146001/mojo/public/bindings/lib/buffer.h#newcode22>
> mojo/public/bindings/lib/**buffer.h:22: class StackBuffer : public Buffer
> {
> On 2013/10/09 17:05:32, darin wrote:
>
>> I was thinking this class might be better named ScratchBuffer. It is
>>
> not just a
>
>> buffer for allocating objects on the stack as it will failover to the
>>
> heap if
>
>> necessary. From the perspective of users, this is the buffer that will
>>
> almost
>
>> always be used. Maybe it should be called ScopedBuffer to make it
>>
> clear that it
>
>> sets up a scope under which objects may be temporarily allocated.
>>
> Thoughts?
>
> I think I like ScratchBuffer more. (To me, ScopedBuffer suggests that
> you could "detach" objects in it for use elsewhere, or detach the
> buffer.)
>
>
Yeah, good point!



>
https://codereview.chromium.**org/23913008/<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.

[darin (slow to review), 2013-10-10 20:21:47]

OK, new file names are in place. PTAL.

I added more to the TODO file.

[darin (slow to review), 2013-10-10 20:23:46]

Please note the use of the Align function and the new
ObjectTraits<T>::ComputeAlignedSizeOf(const T*) functions. These are used to
figure out how much memory to allocate given a struct pointer. It traverses the
object tree to compute a total allocation size. I needed the Align function
since some structs are not multiples of 8 bytes in size. This tells me that
structs should really be padded out to multiples of 8 bytes, but I decided to
make that a follow-up activity.

[viettrungluu, 2013-10-10 20:42:38]

Okay, LGTM (at least to begin with). I say ship it.

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

https://codereview.chromium.org/23913008/diff/176001/mojo/public/system/macro...
mojo/public/system/macros.h:27: TypeName(const TypeName&);               \
Nit: remove extra spaces before \ (or add some to align, I guess).

[darin (slow to review), 2013-10-10 21:04:26]

On Thu, Oct 10, 2013 at 1:42 PM, <viettrungluu@chromium.org> wrote:

> Okay, LGTM (at least to begin with). I say ship it.
>
>
> https://codereview.chromium.**org/23913008/diff/176001/mojo/**
>
public/system/macros.h<https://codereview.chromium.org/23913008/diff/176001/mojo/public/system/macros.h>
> File mojo/public/system/macros.h (right):
>
> https://codereview.chromium.**org/23913008/diff/176001/mojo/**
>
public/system/macros.h#**newcode27<https://codereview.chromium.org/23913008/diff/176001/mojo/public/system/macros.h#newcode27>
> mojo/public/system/macros.h:**27: TypeName(const TypeName&);
> \
> Nit: remove extra spaces before \ (or add some to align, I guess).
>
>
Thanks, good catch.

-Darin



>
https://codereview.chromium.**org/23913008/<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.

[commit-bot: I haz the power, 2013-10-11 07:48:48]

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

[commit-bot: I haz the power, 2013-10-11 10:52:35]

Change committed as 228153