Generate a templated Clone method for all mojo structs.

Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.

Committed: https://crrev.com/fb981f58c60cd22364da2f8f5b4171f45edbe1fb
Cr-Commit-Position: refs/heads/master@{#396608}

[Sam McNally, 2016-05-25 05:58:32]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-05-25 07:06:25]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-05-26 01:17:24]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-05-26 01:17:26]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-05-26 01:17:28]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-05-26 01:17:29]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-05-26 01:17:32]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-05-26 01:17:34]

Patchset #1 (id:140001) has been deleted

[Sam McNally, 2016-05-26 01:17:36]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2016-05-26 01:17:38]

Patchset #1 (id:180001) has been deleted

[Sam McNally, 2016-05-26 01:17:40]

Patchset #1 (id:200001) has been deleted

[Sam McNally, 2016-05-26 01:17:42]

Patchset #1 (id:220001) has been deleted

[Sam McNally, 2016-05-26 03:13:03]

Description was changed from

==========
Make structs containing native types cloneable if the native type is.
==========

to

==========
Make structs containing native types cloneable if the native types are.
==========

[Sam McNally, 2016-05-26 04:06:26]

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

[yzshen1, 2016-05-26 16:24:14]

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:21:
{%-     if field.kind|is_object_kind and not field.kind|is_string_kind and not
field.kind|is_typemapped_kind%}
style nit: 
- one space before %}
- break the line into 2 lines so that it doesn't exceed 80 chars. (I know we
don't strictly follow this rule in tmpl files, but nice to do it whenever
possible.)

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:141: lambda kind :
IsTypemappedKind(kind) and ShouldPassParamByValue(kind))
I feel that the cloneable check should probably be reconsidered. 

1) this method is used in .tmpl files to determine whether we generate a Clone()
function for the standard auto-generated struct wrapper for |kind|. Even if
|kind| itself is typemapped and should be passed-by-value, it doesn't mean that
the auto-generated class for |kind| shouldn't have a Clone() method.

Example:
//mojom
struct Foo { int32 value; }

The auto-generated Foo class has a Clone() method.
With the current logic, if we map Foo to CustomFoo which is pass-by-value, the
Foo class's Clone() method will disappear. But to me it shouldn't be affected by
the typemapping at all (unless it has fields that are typemapped to something
that is not cloneable.)

2) |kind| should be passed by value doesn't mean it cannot be cloned. All
auto-generated struct wrappers are passed by value but they are cloneable.

One possible alternative:
- We always generate Clone(), but similar to Equals() we use a template so that
it is only instantiated if it is used. That way the code generator doesn't need
to do the check.
- Support an optional Clone() function in StructTraits. If this function is not
defined, the type is not cloneable (or alternatively, we can fall back to try to
use copy assignment).
  (We probably can do the same to Equals().)

WDYT? Thanks!

[Sam McNally, 2016-05-27 00:17:58]

Patchset #2 (id:260001) has been deleted

[Sam McNally, 2016-05-27 00:22:32]

Patchset #2 (id:280001) has been deleted

[Sam McNally, 2016-05-27 01:46:25]

Patchset #2 (id:300001) has been deleted

[Sam McNally, 2016-05-27 02:55:08]

Description was changed from

==========
Make structs containing native types cloneable if the native types are.
==========

to

==========
Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.
==========

[Sam McNally, 2016-05-27 02:57:40]

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:21:
{%-     if field.kind|is_object_kind and not field.kind|is_string_kind and not
field.kind|is_typemapped_kind%}
On 2016/05/26 16:24:14, yzshen1 wrote:
> style nit: 
> - one space before %}
> - break the line into 2 lines so that it doesn't exceed 80 chars. (I know we
> don't strictly follow this rule in tmpl files, but nice to do it whenever
> possible.)

Done.

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/2012693002/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:141: lambda kind :
IsTypemappedKind(kind) and ShouldPassParamByValue(kind))
On 2016/05/26 16:24:14, yzshen1 wrote:
> I feel that the cloneable check should probably be reconsidered. 
> 
> 1) this method is used in .tmpl files to determine whether we generate a
Clone()
> function for the standard auto-generated struct wrapper for |kind|. Even if
> |kind| itself is typemapped and should be passed-by-value, it doesn't mean
that
> the auto-generated class for |kind| shouldn't have a Clone() method.
> 
> Example:
> //mojom
> struct Foo { int32 value; }
> 
> The auto-generated Foo class has a Clone() method.
> With the current logic, if we map Foo to CustomFoo which is pass-by-value, the
> Foo class's Clone() method will disappear. But to me it shouldn't be affected
by
> the typemapping at all (unless it has fields that are typemapped to something
> that is not cloneable.)
> 
> 2) |kind| should be passed by value doesn't mean it cannot be cloned. All
> auto-generated struct wrappers are passed by value but they are cloneable.
> 
> One possible alternative:
> - We always generate Clone(), but similar to Equals() we use a template so
that
> it is only instantiated if it is used. That way the code generator doesn't
need
> to do the check.
> - Support an optional Clone() function in StructTraits. If this function is
not
> defined, the type is not cloneable (or alternatively, we can fall back to try
to
> use copy assignment).
>   (We probably can do the same to Equals().)
> 
> WDYT? Thanks!

Done for structs.

[yzshen1, 2016-05-27 21:39:55]

LGTM

Would be nice if we do the same for unions. But that could be a future change.

[Sam McNally, 2016-05-27 23:32:58]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2016-05-27 23:33:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2012693002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2012693002/320001

[commit-bot: I haz the power, 2016-05-28 00:18:48]

Description was changed from

==========
Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.
==========

to

==========
Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.
==========

[commit-bot: I haz the power, 2016-05-28 00:18:49]

Committed patchset #2 (id:320001)

[commit-bot: I haz the power, 2016-05-28 00:20:32]

Description was changed from

==========
Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.
==========

to

==========
Generate a templated Clone method for all mojo structs.

Previously, Clone methods were only generated for mojo structs where all
fields were cloneable. However, for typemapped types, the bindings
generator is not aware of whether the native type is copyable and so it
treated all typemapped types as not cloneable.

This changes the Clone method to be templated so it is only compiled if
used so all mojo structs can contain a Clone method that will be usable
only if all fields are cloneable.

Committed: https://crrev.com/fb981f58c60cd22364da2f8f5b4171f45edbe1fb
Cr-Commit-Position: refs/heads/master@{#396608}
==========

[commit-bot: I haz the power, 2016-05-28 00:20:33]

Patchset 2 (id:??) landed as
https://crrev.com/fb981f58c60cd22364da2f8f5b4171f45edbe1fb
Cr-Commit-Position: refs/heads/master@{#396608}