C bindings: Implement _DeepCopy() & some unittests.

C bindings: Implement _DeepCopy() & some unittests.

This generates/implements DeepCopy(), which is a way to recursively copy
a struct, and adversely move all the handles into a new copy.

Comes with some unittests, but not all of them.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/700f89aa67c60578c8d2cd9712fc2cd5f43a451c

[vardhan, 2016-08-01 22:41:37]

vardhan@google.com changed reviewers:
+ viettrungluu@chromium.org

[vardhan, 2016-08-01 22:41:38]

ptal

[viettrungluu, 2016-08-02 20:15:06]

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/array.h (right):

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/array.h:124: // |buffer|: A mojom buffer used to allocate
space for the new array.
What's the size/space requirement on |buffer|?

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/array.c (right):

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/array.c:229: MojomBuffer_Allocate(buffer,
in_array->num_bytes);
E.g., this could fail. Though at least here you can still bail (and return null)
without a problem.

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/array.c:242: MojomType_DispatchDeepCopy(buffer,
in_type_desc->elem_type,
Presumably, this could fail too. But that would pose a bigger problem, since you
can't cleanly bail here if you're modifying in_array (e.g., to "move" handles).

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
File mojo/public/c/bindings/struct.h (right):

https://codereview.chromium.org/2200843002/diff/20001/mojo/public/c/bindings/...
mojo/public/c/bindings/struct.h:98: // |buffer|: A mojom buffer used to allocate
space for the new struct.
(Same questions about the amount of space required and failures apply here and
elsewhere too.)

[vardhan, 2016-08-02 23:24:02]

Patchset #3 (id:40001) has been deleted

[vardhan, 2016-08-02 23:24:10]

Patchset #3 (id:60001) has been deleted

[vardhan, 2016-08-02 23:26:36]

PTAL.

btw, there is a little inconsistency between the API for MojomStruct/Array and
Union -- struct/array return a pointer to the new copy (or NULL on failure),
whereas union returns false for failure (and the new copy is an out param). i
don't think its that big a deal though, but let me know if its weird enough to
make consistent.

[viettrungluu, 2016-08-03 16:43:24]

(Making things consistent would be nice, if it's not a lot of trouble.)

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/union.c (right):

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:50: continue;
Should this really be |continue|? Or should it be |break|?

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:81: continue;
"

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:111: continue;
"

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:146: continue;
"

(Or maybe |return true;|?)

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:153: return true;
Hmmm, returning true is a little odd: It means that (module the POD stuff above,
which seems dubious to me) in_union_data->tag is unknown (which, depending on
where in_union_data came from, may just mean that it's an instance of a future
version of the union).

But the result is that you didn't know what to do, and you didn't do a deep
copy, even if you should have.

[vardhan, 2016-08-03 23:11:02]

ptal

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/union.c (right):

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:50: continue;
On 2016/08/03 16:43:24, viettrungluu wrote:
> Should this really be |continue|? Or should it be |break|?

It should be break, thanks!

https://codereview.chromium.org/2200843002/diff/80001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/union.c:153: return true;
On 2016/08/03 16:43:24, viettrungluu wrote:
> Hmmm, returning true is a little odd: It means that (module the POD stuff
above,
> which seems dubious to me) in_union_data->tag is unknown (which, depending on
> where in_union_data came from, may just mean that it's an instance of a future
> version of the union).
> 
> But the result is that you didn't know what to do, and you didn't do a deep
> copy, even if you should have.

My assumption was that it shouldn't fail, but discussed offline: it should fail.

[viettrungluu, 2016-08-03 23:45:28]

lgtm w/hard tab nits

https://codereview.chromium.org/2200843002/diff/120001/mojom/generators/c/tem...
File mojom/generators/c/templates/struct.tmpl.go (right):

https://codereview.chromium.org/2200843002/diff/120001/mojom/generators/c/tem...
mojom/generators/c/templates/struct.tmpl.go:75: if (!MojomStruct_DeepCopy(
I think you have some hard tabs in here.

[vardhan, 2016-08-04 00:01:57]

Description was changed from

==========
C bindings: Implement _DeepCopy() & some unittests.

This generates/implements DeepCopy(), which is a way to recursively copy
a struct, and adversely move all the handles into a new copy.

Comes with some unittests, but not all of them.
==========

to

==========
C bindings: Implement _DeepCopy() & some unittests.

This generates/implements DeepCopy(), which is a way to recursively copy
a struct, and adversely move all the handles into a new copy.

Comes with some unittests, but not all of them.

R=viettrungluu@chromium.org

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

[vardhan, 2016-08-04 00:01:59]

Committed patchset #6 (id:140001) manually as
700f89aa67c60578c8d2cd9712fc2cd5f43a451c (presubmit successful).