Implement unions as members of structs.

Implement unions as members of structs and arrays.
BUG=433947
R=yzshen@chromium.org

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

[azani, 2015-02-13 23:07:25]

azani@chromium.org changed reviewers:
+ yzshen@chromium.org

[azani, 2015-02-13 23:07:25]

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/m...
File mojo/public/tools/bindings/mojom_bindings_generator.py (left):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/m...
mojo/public/tools/bindings/mojom_bindings_generator.py:197:
metavar="GENERATORS",
Please ignore. I will undo before landing.

[yzshen1, 2015-02-17 19:34:36]

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:289: SmallStructPtr
small_struct(SmallStruct::New());
Why do we need this test and ObjectUnionInStruct? It seems they are very
obvious, and other test failures will be triggered if such basic functionality
doesn't work.

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:347: TEST(UnionTest,
ValidationJustWorksForUnionsInStructs) {
Nit: maybe UnionsInStructsValidation?

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:364: TEST(UnionTest,
PodUnionInStructValidationFails) {
Maybe use the same style across multiple test names?

XXXValidation
XXXValidationFailure

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:66: {#---
Unions must be declared first because they can be members of structs #}
Do we need to distinguish between "inlined unions" and "non-inlined unions" just
like what line 72 says for structs? (I haven't tried myself.)

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18: {%-  
    if not kind|is_nullable_kind %}
(Please note that this part has been under significant change recently.
Especially after https://codereview.chromium.org/917333003/. Besides, I am
planning to move tmpl code into C++ template magic. :))

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:28: if
(!{{kind.name}}::Data_::Validate(&object->{{name}}, bounds_checker, false)) {
Do you think it make sense to make |false| a template argument? That seems to be
more consistent with other Validate().

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:17:
{%-   if pf.field.kind|is_union_kind %}
It seems nicer to move this into the "is_object_kind" branch.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:14:
bool claim_memory);
I think it seems nicer to name it |inlined| because "claim memory
 may not be the only thing affected during validation.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:17:
mojo::internal::BoundsChecker* bounds_checker) {
Maybe we should remove this, so that if we don't handle union validation
explicitly (for example, in array/map validation), code compilation will fail?

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:21:
bool is_null() const {
This is not correct: when the union is non-inlined, the first four bytes are the
size of the union, which is 16. (I wish we used 16 for both inlined/non-inlined
case. It is more consistent and less confusing. But qsr has a different opinion,
maybe we should show him the real code?)

Besides, please avoid adding methods if possible, because with this people are
not able to define a union member |is_null| anymore.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:49:
uint32_t flags;
The name |flags| seems confusing. It is the size of the union in non-lined case.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:10: 
unnecessary empty line.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:24: //
If the union is inlined in another structure its memory was already claimed.
nit: I understand 80-char limit is not honored in general in .tmpl files, but
usually people stick to 80-char limits for comments.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:26: if
(claim_memory && !bounds_checker->ClaimMemory(data, sizeof({{class_name}}))) {
For non-inlined case, we need to check that the first four bytes stores the
value 16. (I wish we could do the same for non-null inlined case, too. Please
see my comment in the declaration.)

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:25:
if (*output) {
Unfortunately, we don't initialize |*output|. (For example, if you look at
struct_definition.tmpl, the constructor doesn't do anything about the fields.)
So it may be a random non-zero value.

That is why we have line 49.


As a comparison, we do initialization for the wrapper class, because that one is
user facing.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:33:
result->flags = 0x1;
This is the size for the non-inlined case.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:21:
{%-     if field.kind|is_struct_kind or field.kind|is_array_kind or
field.kind|is_map_kind or field.kind|is_union_kind %}
Maybe we should say "is object but not string", now that the list is getting
longer and longer?

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:70: def
GetCppType(kind):
There are quite a few of GetXXXType(), they all need to properly handle union.

[azani, 2015-02-18 00:27:58]

ptal

I left most of the comments regarding flags/size unanswered since we're still
figuring that out.

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:289: SmallStructPtr
small_struct(SmallStruct::New());
On 2015/02/17 19:34:35, yzshen1 wrote:
> Why do we need this test and ObjectUnionInStruct? It seems they are very
> obvious, and other test failures will be triggered if such basic functionality
> doesn't work.

Artefacts of TDD. Gone now.

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:347: TEST(UnionTest,
ValidationJustWorksForUnionsInStructs) {
On 2015/02/17 19:34:35, yzshen1 wrote:
> Nit: maybe UnionsInStructsValidation?

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/union_unittest.cc:364: TEST(UnionTest,
PodUnionInStructValidationFails) {
On 2015/02/17 19:34:35, yzshen1 wrote:
> Maybe use the same style across multiple test names?
> 
> XXXValidation
> XXXValidationFailure

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:66: {#---
Unions must be declared first because they can be members of structs #}
On 2015/02/17 19:34:35, yzshen1 wrote:
> Do we need to distinguish between "inlined unions" and "non-inlined unions"
just
> like what line 72 says for structs? (I haven't tried myself.)

I don't think so. Unions are never inlined in unions, so that problem shouldn't
come up.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18: {%-  
    if not kind|is_nullable_kind %}
On 2015/02/17 19:34:35, yzshen1 wrote:
> (Please note that this part has been under significant change recently.
> Especially after https://codereview.chromium.org/917333003/. Besides, I am
> planning to move tmpl code into C++ template magic. :))

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:28: if
(!{{kind.name}}::Data_::Validate(&object->{{name}}, bounds_checker, false)) {
On 2015/02/17 19:34:35, yzshen1 wrote:
> Do you think it make sense to make |false| a template argument? That seems to
be
> more consistent with other Validate().

It seems like we would just shift the inconsistency from the argument list to
the template arguments. I am somewhat disinclined to using templates for this
purpose, but only slightly so.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:14:
bool claim_memory);
On 2015/02/17 19:34:35, yzshen1 wrote:
> I think it seems nicer to name it |inlined| because "claim memory
>  may not be the only thing affected during validation.

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:17:
mojo::internal::BoundsChecker* bounds_checker) {
On 2015/02/17 19:34:35, yzshen1 wrote:
> Maybe we should remove this, so that if we don't handle union validation
> explicitly (for example, in array/map validation), code compilation will fail?

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:21:
bool is_null() const {
On 2015/02/17 19:34:35, yzshen1 wrote:
> This is not correct: when the union is non-inlined, the first four bytes are
the
> size of the union, which is 16. (I wish we used 16 for both
inlined/non-inlined
> case. It is more consistent and less confusing. But qsr has a different
opinion,
> maybe we should show him the real code?)

I think I had a different understanding of the outcome of that thread. I thought
we were not going to store the size (since the size is always 16 bytes and just
reserve the space, including for a "not null" flag. (The not null flag could
probably be implemented differently and I would appreciate some advice. Should I
just use a bit mask? Where should I locate the mask?)

> 
> Besides, please avoid adding methods if possible, because with this people are
> not able to define a union member |is_null| anymore.

|is_null| doesn't conflict with union members since this is the _Data class
which doesn't have methods for each member.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl:49:
uint32_t flags;
On 2015/02/17 19:34:35, yzshen1 wrote:
> The name |flags| seems confusing. It is the size of the union in non-lined
case.

See above. I thought we were just reserving the space since the size is set at
16 bytes.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:10: 
On 2015/02/17 19:34:35, yzshen1 wrote:
> unnecessary empty line.

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:24: //
If the union is inlined in another structure its memory was already claimed.
On 2015/02/17 19:34:35, yzshen1 wrote:
> nit: I understand 80-char limit is not honored in general in .tmpl files, but
> usually people stick to 80-char limits for comments.
> 

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:25:
if (*output) {
On 2015/02/17 19:34:35, yzshen1 wrote:
> Unfortunately, we don't initialize |*output|. (For example, if you look at
> struct_definition.tmpl, the constructor doesn't do anything about the fields.)
> So it may be a random non-zero value.
> 
> That is why we have line 49.

The problem I'm trying to solve is again related to inlining.

If we are serializing a free-standing union, we need to allocate some new memory
for the union. But if the union is inlined, then memory has already been
allocated for it.

The code above works when dealing with inlined unions since |*output| is the
location where the union needs to be inlined. Is your concern that if the union
is free-standing, *output might not be initialized to nullptr?

> 
> 
> As a comparison, we do initialization for the wrapper class, because that one
is
> user facing.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:21:
{%-     if field.kind|is_struct_kind or field.kind|is_array_kind or
field.kind|is_map_kind or field.kind|is_union_kind %}
On 2015/02/17 19:34:35, yzshen1 wrote:
> Maybe we should say "is object but not string", now that the list is getting
> longer and longer?

Done.

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:70: def
GetCppType(kind):
On 2015/02/17 19:34:35, yzshen1 wrote:
> There are quite a few of GetXXXType(), they all need to properly handle union.

If that's ok with you, I would rather add them as they become needed. Figuring
out exactly what the right type to return is not always trivial and I would
rather do so in the context of making the feature work rather than more
abstractly up front.

[azani, 2015-02-24 23:42:32]

ptal

size isssue is fixed.

I added support for arrays.

[yzshen1, 2015-02-25 21:07:01]

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/923033003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:70: def
GetCppType(kind):
On 2015/02/18 00:27:58, azani wrote:
> On 2015/02/17 19:34:35, yzshen1 wrote:
> > There are quite a few of GetXXXType(), they all need to properly handle
union.
> 
> If that's ok with you, I would rather add them as they become needed. Figuring
> out exactly what the right type to return is not always trivial and I would
> rather do so in the context of making the feature work rather than more
> abstractly up front.

I am a little concerned to leave these set of functions in inconsistent state.
Hmm... :/

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:51: bool is_union =
internal::IsWrappedUnionPtr<E>::value>
nit: internal:: is not needed.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:218: template <typename U,
typename U_Data>
nit: Please comment about what is this specialization for.
(It would be nice if we do the same for others as well.)

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:234:
Serialize_(input[i].Pass(), buf, &result);
I think maybe we should make a different SerializeUnion_ signature for unions,
which takes a boolean indicating whether |*result| is already allocated.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:235: }
Please look at line 168: we need to do similar things here (DCHECK that an
unexpected null pointer in an array which is supported to store non-null
pointers.)

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:248: /*
If something is not useful, please remove it instead of commenting it out.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:97: struct
IsWrappedUnionPtr<StructPtr<S>> {
I feel that you could use IsUnionDataType and WrapperTraits  to implement this
IsWrappedUnionPtr, instead of doing the overloaded Test() trick again.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:108: struct
IsWrappedUnionPtr<InlinedStructPtr<S>> {
ditto.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:141: typedef typename S::Data_
DataType;
I looked at array_serialization, and didn't see why we need it to be S::Data_
instead of S::Data_* (but I didn't look very hard). Would you please point me to
it?

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:290: TEST(UnionTest,
StringValidateStringValidation) {
nit: I cannot parse this test name...

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:479: 0, false,
mojo::internal::NoValidateParams>>(array.Pass(), &buf, &data);
false -> true, meaning that elements are nullable.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:490: /*
Please remove any code if it is not needed. (Or maybe you should uncomment?)

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:180: {%- 
   if pf.field.kind|is_union_kind %}
Could you please move the "is_union_kind" part into the "is_object_kind" part?

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:210: {%- 
     if kind|is_union_kind %}
ditto

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:260: {%- 
 if kind|is_union_kind %}
ditto.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:261:
internal::{{pf.field.kind.name}}_Data* {{pf.field.name}}_ptr =
&result->{{pf.field.name}};
Please use {{input_field}}, {{buffer}} and {{output}} instead of "input->xxx",
"buf" and "result".

Besides, {{name}} and {{kind}} instead of {{pf.field.name}} and
{{pf.field.kind}}.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:326: {%- 
   if kind|is_union_kind %}
Please move this if into the if for "is_object_kind".

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:327:
Deserialize_(&input->{{pf.field.name}}, &result->{{pf.field.name}});
Please use {{output_field}}, {{input}}.

Besides, {{name}} instead of {{pf.field.name}}

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
For non-inlined union, this is incorrect. :/

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:24:
if (*output) {
Please consider the change that we discussed offline.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:29:
if (input) {
if input is null, we need to either set |output| to nullptr (non-inlined) or set
the contents to all-zero (inlined).

[azani, 2015-03-03 00:42:51]

azani@chromium.org changed reviewers:
+ jamesr@chromium.org, viettrungluu@chromium.org
- yzshen@chromium.org

[azani, 2015-03-03 00:44:16]

Hey James and Thrung,

Is either of you able to review my change or suggest someone else since yzshen
is out on paternity leave?

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:51: bool is_union =
internal::IsWrappedUnionPtr<E>::value>
On 2015/02/25 21:07:00, yzshen1 wrote:
> nit: internal:: is not needed.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:218: template <typename U,
typename U_Data>
On 2015/02/25 21:06:59, yzshen1 wrote:
> nit: Please comment about what is this specialization for.
> (It would be nice if we do the same for others as well.)

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:234:
Serialize_(input[i].Pass(), buf, &result);
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> I think maybe we should make a different SerializeUnion_ signature for unions,
> which takes a boolean indicating whether |*result| is already allocated.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:235: }
On 2015/02/25 21:07:00, yzshen1 wrote:
> Please look at line 168: we need to do similar things here (DCHECK that an
> unexpected null pointer in an array which is supported to store non-null
> pointers.)

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:248: /*
On 2015/02/25 21:07:00, yzshen1 wrote:
> If something is not useful, please remove it instead of commenting it out.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:97: struct
IsWrappedUnionPtr<StructPtr<S>> {
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> I feel that you could use IsUnionDataType and WrapperTraits  to implement this
> IsWrappedUnionPtr, instead of doing the overloaded Test() trick again.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:108: struct
IsWrappedUnionPtr<InlinedStructPtr<S>> {
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> ditto.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/bindings_internal.h:141: typedef typename S::Data_
DataType;
On 2015/02/25 21:07:00, yzshen1 wrote:
> I looked at array_serialization, and didn't see why we need it to be S::Data_
> instead of S::Data_* (but I didn't look very hard). Would you please point me
to
> it?

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:290: TEST(UnionTest,
StringValidateStringValidation) {
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> nit: I cannot parse this test name...

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:479: 0, false,
mojo::internal::NoValidateParams>>(array.Pass(), &buf, &data);
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> false -> true, meaning that elements are nullable.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:180: {%- 
   if pf.field.kind|is_union_kind %}
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> Could you please move the "is_union_kind" part into the "is_object_kind" part?

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:210: {%- 
     if kind|is_union_kind %}
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> ditto

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:260: {%- 
 if kind|is_union_kind %}
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> ditto.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:261:
internal::{{pf.field.kind.name}}_Data* {{pf.field.name}}_ptr =
&result->{{pf.field.name}};
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> Please use {{input_field}}, {{buffer}} and {{output}} instead of "input->xxx",
> "buf" and "result".
> 
> Besides, {{name}} and {{kind}} instead of {{pf.field.name}} and
> {{pf.field.kind}}.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:326: {%- 
   if kind|is_union_kind %}
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> Please move this if into the if for "is_object_kind".

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:327:
Deserialize_(&input->{{pf.field.name}}, &result->{{pf.field.name}});
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> Please use {{output_field}}, {{input}}.
> 
> Besides, {{name}} instead of {{pf.field.name}}

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> For non-inlined union, this is incorrect. :/

This only applies if we have a pointer to a union which should only be the case
when it's a union within a union. I've added a new function to deal with that
situation.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:24:
if (*output) {
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> Please consider the change that we discussed offline.

Done.

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:29:
if (input) {
On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> if input is null, we need to either set |output| to nullptr (non-inlined) or
set
> the contents to all-zero (inlined).

It's not an inlined vs not inlined issue. It's "pointed to by a pointer" vs not.
Since this only applies to unions with union members, I would rather tackle that
when I add support for unions being members of unions.

[viettrungluu, 2015-03-04 20:58:13]

OK, I haven't reviewed the .tmpl and the .py changes yet, but here are some
comments for the .cc/.h files.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_internal.h:288: template <typename T,
typename Params, bool isUnion>
nit: (FYI) camelCase is never used in (our) C++

Anyhow, I don't think this extra parameter is needed. I seems to me that if you
can determine from T if it's a union data type (e.g., in the above callsite),
you should be able to write template specializations appropriately too.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:226: size_t size =
sizeof(Array_Data<U_Data>);
I don't understand why this doesn't account for the size of the inline part of
the data (i.e., why there isn't a "+ input.size() * <something>" [maybe it
should be "+ Align(...)", something which is missing from the
immediately-preceding specialization]).

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:227: for (size_t i = 0; i <
input.size(); ++i) {
nit: Other one-line for statements in this file don't have braces, so for
consistency you should omit them here too.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:228: size +=
GetSerializedSize_(input[i]);
(I suppose might include the inline part, but that's not super-obvious.)

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:252:
Deserialize_(&input->at(i), &element);
Why don't you just do &result[i], instead of going through |element| and the
.Pass()ing?

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/template_util.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/template_util.h:104: typedef T type;
This is probably right (since it matches[*] the C++11 remove_pointer), but have
you double-checked that it doesn't have any inadvertent effects?

[*] Well, we still don't handle const pointers correctly (nor volatile).

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:433: TEST(UnionTest,
PodUnionInArray) {
Probably for consistency, tests of arrays of unions should probably be in
array_unittest.cc.

[azani, 2015-03-05 23:59:23]

azani@chromium.org changed reviewers:
- jamesr@chromium.org

[azani, 2015-03-05 23:59:25]

Thanks for the review. ptal

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_internal.h:288: template <typename T,
typename Params, bool isUnion>
On 2015/03/04 20:58:13, viettrungluu wrote:
> nit: (FYI) camelCase is never used in (our) C++
> 
> Anyhow, I don't think this extra parameter is needed. I seems to me that if
you
> can determine from T if it's a union data type (e.g., in the above callsite),
> you should be able to write template specializations appropriately too.

I've updated the case issue. I could be wrong, but I cannot figure out a way to
write the specialization without this extra parameter.

I tried

ValidateCaller<typename EnableIf<IsUnionDataType<T>::value>::type, Params>

However that does not work because T cannot be deduced. Also, I think there may
be some value in the clarity of having an extra parameter rather than something
more complicated. But maybe you have something I could not think of in mind.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:226: size_t size =
sizeof(Array_Data<U_Data>);
On 2015/03/04 20:58:13, viettrungluu wrote:
> I don't understand why this doesn't account for the size of the inline part of
> the data (i.e., why there isn't a "+ input.size() * <something>" [maybe it
> should be "+ Align(...)", something which is missing from the
> immediately-preceding specialization]).

See lower.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:227: for (size_t i = 0; i <
input.size(); ++i) {
On 2015/03/04 20:58:13, viettrungluu wrote:
> nit: Other one-line for statements in this file don't have braces, so for
> consistency you should omit them here too.

Done.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:228: size +=
GetSerializedSize_(input[i]);
On 2015/03/04 20:58:13, viettrungluu wrote:
> (I suppose might include the inline part, but that's not super-obvious.)

It does. I've added a comment to clarify.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_serialization.h:252:
Deserialize_(&input->at(i), &element);
On 2015/03/04 20:58:13, viettrungluu wrote:
> Why don't you just do &result[i], instead of going through |element| and the
> .Pass()ing?

Done.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/template_util.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/template_util.h:104: typedef T type;
On 2015/03/04 20:58:13, viettrungluu wrote:
> This is probably right (since it matches[*] the C++11 remove_pointer), but
have
> you double-checked that it doesn't have any inadvertent effects?
> 
> [*] Well, we still don't handle const pointers correctly (nor volatile).

Yes. Grepping revealed only one use of RemovePointer and this is fine.

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/union_unittest.cc:433: TEST(UnionTest,
PodUnionInArray) {
On 2015/03/04 20:58:13, viettrungluu wrote:
> Probably for consistency, tests of arrays of unions should probably be in
> array_unittest.cc.

Done.

[azani, 2015-03-16 18:13:41]

friendly ping

[azani, 2015-03-16 18:13:45]

friendly ping

[viettrungluu, 2015-03-17 20:08:03]

Some ideas re: templates....

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_internal.h:288: template <typename T,
typename Params, bool isUnion>
On 2015/03/05 23:59:24, azani wrote:
> On 2015/03/04 20:58:13, viettrungluu wrote:
> > nit: (FYI) camelCase is never used in (our) C++
> > 
> > Anyhow, I don't think this extra parameter is needed. I seems to me that if
> you
> > can determine from T if it's a union data type (e.g., in the above
callsite),
> > you should be able to write template specializations appropriately too.
> 
> I've updated the case issue. I could be wrong, but I cannot figure out a way
to
> write the specialization without this extra parameter.
> 
> I tried
> 
> ValidateCaller<typename EnableIf<IsUnionDataType<T>::value>::type, Params>
> 
> However that does not work because T cannot be deduced. Also, I think there
may
> be some value in the clarity of having an extra parameter rather than
something
> more complicated. But maybe you have something I could not think of in mind.

Maybe something like the example from
http://en.cppreference.com/w/cpp/types/enable_if would work?

// the partial specialization of A is enabled via a template parameter
template<class T, class Enable = void>
class A {}; // primary template
 
template<class T>
class A<T, typename std::enable_if<std::is_floating_point<T>::value >::type> {
}; // specialization for floating point types

There are other ways you could probably make it work. E.g., I believe you could
add something like:

private:
 typedef typename EnableIf<...>::type maybe_enable_;

to the specialized |struct ValidateCaller|.

The point is that you only need to make the substitution fail, not that the
substitution need actually be meaningful.

(Note that the substitution must be in the declaration, and not in a definition,
since the latter are only expanded on use. I.e., putting such a typedef inside
Run() wouldn't work, since things would only blow up when someone tried to call
Run().)

[azani, 2015-03-18 01:04:39]

ptal

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/923033003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_internal.h:288: template <typename T,
typename Params, bool isUnion>
On 2015/03/17 20:08:02, viettrungluu wrote:
> On 2015/03/05 23:59:24, azani wrote:
> > On 2015/03/04 20:58:13, viettrungluu wrote:
> > > nit: (FYI) camelCase is never used in (our) C++
> > > 
> > > Anyhow, I don't think this extra parameter is needed. I seems to me that
if
> > you
> > > can determine from T if it's a union data type (e.g., in the above
> callsite),
> > > you should be able to write template specializations appropriately too.
> > 
> > I've updated the case issue. I could be wrong, but I cannot figure out a way
> to
> > write the specialization without this extra parameter.
> > 
> > I tried
> > 
> > ValidateCaller<typename EnableIf<IsUnionDataType<T>::value>::type, Params>
> > 
> > However that does not work because T cannot be deduced. Also, I think there
> may
> > be some value in the clarity of having an extra parameter rather than
> something
> > more complicated. But maybe you have something I could not think of in mind.
> 
> Maybe something like the example from
> http://en.cppreference.com/w/cpp/types/enable_if would work?
> 
> // the partial specialization of A is enabled via a template parameter
> template<class T, class Enable = void>
> class A {}; // primary template
>  
> template<class T>
> class A<T, typename std::enable_if<std::is_floating_point<T>::value >::type> {
> }; // specialization for floating point types
> 
> There are other ways you could probably make it work. E.g., I believe you
could
> add something like:
> 
> private:
>  typedef typename EnableIf<...>::type maybe_enable_;
> 
> to the specialized |struct ValidateCaller|.
> 
> The point is that you only need to make the substitution fail, not that the
> substitution need actually be meaningful.
> 
> (Note that the substitution must be in the declaration, and not in a
definition,
> since the latter are only expanded on use. I.e., putting such a typedef inside
> Run() wouldn't work, since things would only blow up when someone tried to
call
> Run().)

I have not been able to get these to work.

Putting a typedef in the struct definition doesn't work at all. It appears that
isn't taken into account when doing template specialization partial ordering.

Using EnableIf could possibly be made to work. However, the problem is that if I
use EnableIf only on the union version of ValidateCaller, it creates an
ambiguity between the union version and the struct version. 

But if I also use EnableIf on the struct version of ValidateCaller also, then
another ambiguity arises between the struct version and the array version. I
imagine it can probably be made to work somehow, but IMO it looks like it will
require complicated template magic and so the version I already have is likely
to be a bit easier to understand for readers.

We can chat about it in person tomorrow if you want.

[yzshen1, 2015-03-26 07:30:13]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2015-03-26 07:30:15]

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:29:
if (input) {
On 2015/03/03 00:44:16, azani wrote:
> On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> > if input is null, we need to either set |output| to nullptr (non-inlined) or
> set
> > the contents to all-zero (inlined).
> 
> It's not an inlined vs not inlined issue. It's "pointed to by a pointer" vs
not.
> Since this only applies to unions with union members, I would rather tackle
that
> when I add support for unions being members of unions.

I am not sure I understand your comment.

Let's say we have the following definition:
union Foo {};
struct Bar {
  Foo foo;
};

So the Serialize_() of Bar will call into SerializeUnion_() of Foo, passing a
FooPtr as input.

Because Foo_Data is part of the memory of Bar_Data (i.e., inlined), if the given
FooPtr is null, what we should do is to set the Foo_Data range of memory to
all-zero, right?

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:52: IsUnionDataType<typename
internal::RemovePointer<F>::type>::value>
nit: internal:: is not needed, is it?

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:225: for (size_t i = 0; i <
input.size(); ++i)
Please add {}. The body of the for-statement are multiple lines.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:432: TEST_F(StructTest,
PodUnionInStructSerialization) {
nit: the naming is not very consistent within this file. The existing tests use
"Serialization_XXX", "Construction_XXX", "Versioning_XXX"

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:1:
{%- macro declare_struct_serialization(name) %}
now that union and struct don't share the same declaration, maybe we don't have
to make them macros. Besides, we could put them in
{struct,union}_serialization_declaration.tmpl

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:10:
size_t GetSerializedSize_(const {{name}}Ptr& input);
What is the relationship between this one and the following tow functions? Maybe
you could add some comments.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:12:
size_t GetStandaloneSerializeSize_(const {{name}}Ptr& input);
This one is not defined in union_serialization_definition.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:47:
&object->{{name}}, bounds_checker, true)) {
wrong indent?

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:177:
{%-     if kind|is_union_kind %}
this if-elif seems weird. you could do something similar to line 151.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:180: {%- 
   if pf.field.kind|is_object_kind %}
This macro (and some others) are gone.

There might be merge issues when you synced code. I noticed that
struct_definition.tmpl has the new code, which indicates that you have updated
the code; but this file still contains the old code.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:334: {%- 
   if kind|is_union_kind %}
Please move this if into the if for "is_object_kind".

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:335:
Deserialize_(&input->{{pf.field.name}}, &result->{{pf.field.name}});
Please use {{output_field}}, {{input}}.

Besides, {{name}} instead of {{pf.field.name}}

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
I think this is incorrect.
When a struct calculates its size, it will call its union member's
GetSerializedSize_(). In that case, sizeof(internal::{{union.name}}_Data) is
caculated twice (because it is inlined).

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:20:
size_t GetUnionInUnionSerializedSize_(const {{union.name}}Ptr& input) {
The names seem confusing.
Maybe we should just follow what we do for SerializeUnion_(), i.e., adding a
|inlined| parameter?

[azani, 2015-03-26 22:27:40]

ptal

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:29:
if (input) {
On 2015/03/26 07:30:14, yzshen1 wrote:
> On 2015/03/03 00:44:16, azani wrote:
> > On 2015/02/25 21:07:00, yzshen1 (OOO Mar 2 - 22) wrote:
> > > if input is null, we need to either set |output| to nullptr (non-inlined)
or
> > set
> > > the contents to all-zero (inlined).
> > 
> > It's not an inlined vs not inlined issue. It's "pointed to by a pointer" vs
> not.
> > Since this only applies to unions with union members, I would rather tackle
> that
> > when I add support for unions being members of unions.
> 
> I am not sure I understand your comment.
> 
> Let's say we have the following definition:
> union Foo {};
> struct Bar {
>   Foo foo;
> };
> 
> So the Serialize_() of Bar will call into SerializeUnion_() of Foo, passing a
> FooPtr as input.
> 
> Because Foo_Data is part of the memory of Bar_Data (i.e., inlined), if the
given
> FooPtr is null, what we should do is to set the Foo_Data range of memory to
> all-zero, right?
> 

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:52: IsUnionDataType<typename
internal::RemovePointer<F>::type>::value>
On 2015/03/26 07:30:14, yzshen1 wrote:
> nit: internal:: is not needed, is it?

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:225: for (size_t i = 0; i <
input.size(); ++i)
On 2015/03/26 07:30:14, yzshen1 wrote:
> Please add {}. The body of the for-statement are multiple lines.

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:432: TEST_F(StructTest,
PodUnionInStructSerialization) {
On 2015/03/26 07:30:14, yzshen1 wrote:
> nit: the naming is not very consistent within this file. The existing tests
use
> "Serialization_XXX", "Construction_XXX", "Versioning_XXX"
> 

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:1:
{%- macro declare_struct_serialization(name) %}
On 2015/03/26 07:30:14, yzshen1 wrote:
> now that union and struct don't share the same declaration, maybe we don't
have
> to make them macros. Besides, we could put them in
> {struct,union}_serialization_declaration.tmpl

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:10:
size_t GetSerializedSize_(const {{name}}Ptr& input);
On 2015/03/26 07:30:14, yzshen1 wrote:
> What is the relationship between this one and the following tow functions?
Maybe
> you could add some comments.

Next two are gone. I ended up not needing them in this CL.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/serialization_macros.tmpl:12:
size_t GetStandaloneSerializeSize_(const {{name}}Ptr& input);
On 2015/03/26 07:30:14, yzshen1 wrote:
> This one is not defined in union_serialization_definition.

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:47:
&object->{{name}}, bounds_checker, true)) {
On 2015/03/26 07:30:14, yzshen1 wrote:
> wrong indent?

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:177:
{%-     if kind|is_union_kind %}
On 2015/03/26 07:30:14, yzshen1 wrote:
> this if-elif seems weird. you could do something similar to line 151.

Won't work because then it falls to the "else" clause which is not correct. I
added a TODO to reflect the "work-in-progress" nature of this state of affairs.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:180: {%- 
   if pf.field.kind|is_object_kind %}
On 2015/03/26 07:30:15, yzshen1 wrote:
> This macro (and some others) are gone.
> 
> There might be merge issues when you synced code. I noticed that
> struct_definition.tmpl has the new code, which indicates that you have updated
> the code; but this file still contains the old code.

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:334: {%- 
   if kind|is_union_kind %}
On 2015/03/26 07:30:15, yzshen1 wrote:
> Please move this if into the if for "is_object_kind".

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:335:
Deserialize_(&input->{{pf.field.name}}, &result->{{pf.field.name}});
On 2015/03/26 07:30:15, yzshen1 wrote:
> Please use {{output_field}}, {{input}}.
> 
> Besides, {{name}} instead of {{pf.field.name}}

Done.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/03/26 07:30:15, yzshen1 wrote:
> I think this is incorrect.
> When a struct calculates its size, it will call its union member's
> GetSerializedSize_(). In that case, sizeof(internal::{{union.name}}_Data) is
> caculated twice (because it is inlined).

Removed that function because it was unused and therefore untested.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:20:
size_t GetUnionInUnionSerializedSize_(const {{union.name}}Ptr& input) {
On 2015/03/26 07:30:15, yzshen1 wrote:
> The names seem confusing.
> Maybe we should just follow what we do for SerializeUnion_(), i.e., adding a
> |inlined| parameter?

It was unused, so this was premature anyways.

[yzshen1, 2015-03-27 16:26:33]

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/03/26 22:27:40, azani wrote:
> On 2015/03/26 07:30:15, yzshen1 wrote:
> > I think this is incorrect.
> > When a struct calculates its size, it will call its union member's
> > GetSerializedSize_(). In that case, sizeof(internal::{{union.name}}_Data) is
> > caculated twice (because it is inlined).
> 
> Removed that function because it was unused and therefore untested.

It seems the problem is not addressed. You could try out the following:

union Foo { uint32 a; };
struct Bar {
  Foo foo;
};

What is the serialized size of a non-null Bar instance?
It should be 24, right? But it seems your code will return a bigger size.

https://codereview.chromium.org/923033003/diff/180001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:255: // Serialization test of
a struct with a union of plain of data.
nit: plain *old* data? (and line 273)

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:50:
data.unknown = 0;
nit: 0U?

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:26:
result = internal::{{union.name}}_Data::New(buf);
If it is non-inlined, and input is nullptr, you don't need to allocate |result|,
right?

How about 

if (input) {
  // declare |result|.
  ...
  *output = result;
} else {
  *output = nullptr;
}

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:68:
"$generator_root/generators/cpp_templates/union_serialization_declaration.tmpl",
alphabetically, please.

[azani, 2015-03-27 20:50:14]

ptal

https://codereview.chromium.org/923033003/diff/180001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:255: // Serialization test of
a struct with a union of plain of data.
On 2015/03/27 16:26:33, yzshen1 wrote:
> nit: plain *old* data? (and line 273)

Done.

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl:50:
data.unknown = 0;
On 2015/03/27 16:26:33, yzshen1 wrote:
> nit: 0U?

Done.

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:26:
result = internal::{{union.name}}_Data::New(buf);
On 2015/03/27 16:26:33, yzshen1 wrote:
> If it is non-inlined, and input is nullptr, you don't need to allocate
|result|,
> right?
> 
> How about 
> 
> if (input) {
>   // declare |result|.
>   ...
>   *output = result;
> } else {
>   *output = nullptr;
> }

I went with something a bit different, but same idea.

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/923033003/diff/180001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:68:
"$generator_root/generators/cpp_templates/union_serialization_declaration.tmpl",
On 2015/03/27 16:26:33, yzshen1 wrote:
> alphabetically, please.

Done.

[yzshen1, 2015-03-27 21:52:42]

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/03/27 16:26:33, yzshen1 wrote:
> On 2015/03/26 22:27:40, azani wrote:
> > On 2015/03/26 07:30:15, yzshen1 wrote:
> > > I think this is incorrect.
> > > When a struct calculates its size, it will call its union member's
> > > GetSerializedSize_(). In that case, sizeof(internal::{{union.name}}_Data)
is
> > > caculated twice (because it is inlined).
> > 
> > Removed that function because it was unused and therefore untested.
> 
> It seems the problem is not addressed. You could try out the following:
> 
> union Foo { uint32 a; };
> struct Bar {
>   Foo foo;
> };
> 
> What is the serialized size of a non-null Bar instance?
> It should be 24, right? But it seems your code will return a bigger size.

This comment is not addressed.

[azani, 2015-03-30 22:21:03]

sorry, last comment addressed and fixed.

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/03/27 21:52:42, yzshen1 wrote:
> On 2015/03/27 16:26:33, yzshen1 wrote:
> > On 2015/03/26 22:27:40, azani wrote:
> > > On 2015/03/26 07:30:15, yzshen1 wrote:
> > > > I think this is incorrect.
> > > > When a struct calculates its size, it will call its union member's
> > > > GetSerializedSize_(). In that case,
sizeof(internal::{{union.name}}_Data)
> is
> > > > caculated twice (because it is inlined).
> > > 
> > > Removed that function because it was unused and therefore untested.
> > 
> > It seems the problem is not addressed. You could try out the following:
> > 
> > union Foo { uint32 a; };
> > struct Bar {
> >   Foo foo;
> > };
> > 
> > What is the serialized size of a non-null Bar instance?
> > It should be 24, right? But it seems your code will return a bigger size.
> 
> This comment is not addressed.

Sorry, I finally understood what you meant. I've added GetInlinedSerializeSize_
which does not count the 16 bytes to store the size, tag and data. It's
currently only used with structs. (Arrays count both the inline and not-inline
part at the same time.)

[yzshen1, 2015-03-30 22:55:19]

LGTM with one last comment.

Thanks!

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/923033003/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:4:
return size;
On 2015/03/30 22:21:03, azani wrote:
> On 2015/03/27 21:52:42, yzshen1 wrote:
> > On 2015/03/27 16:26:33, yzshen1 wrote:
> > > On 2015/03/26 22:27:40, azani wrote:
> > > > On 2015/03/26 07:30:15, yzshen1 wrote:
> > > > > I think this is incorrect.
> > > > > When a struct calculates its size, it will call its union member's
> > > > > GetSerializedSize_(). In that case,
> sizeof(internal::{{union.name}}_Data)
> > is
> > > > > caculated twice (because it is inlined).
> > > > 
> > > > Removed that function because it was unused and therefore untested.
> > > 
> > > It seems the problem is not addressed. You could try out the following:
> > > 
> > > union Foo { uint32 a; };
> > > struct Bar {
> > >   Foo foo;
> > > };
> > > 
> > > What is the serialized size of a non-null Bar instance?
> > > It should be 24, right? But it seems your code will return a bigger size.
> > 
> > This comment is not addressed.
> 
> Sorry, I finally understood what you meant. I've added
GetInlinedSerializeSize_
> which does not count the 16 bytes to store the size, tag and data. It's
> currently only used with structs. (Arrays count both the inline and not-inline
> part at the same time.)

It seems better to follow the same style as SerializeUnion_(), i.e., adding a
bool |inlined| parameter:
GetSerializedSize_(const FooPtr& input, bool inlined);

[azani, 2015-03-31 00:23:40]

Committed patchset #17 (id:320001) manually as
bb7095ab2314ef45e6350528c98b2504efbabc53 (presubmit successful).