New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 616527 link

Starred by 1 user
Status: Fixed
Owner:
█ yzshen@chromium.org
Last visit > 30 days ago
Closed: Jun 2016
Cc:
█ roc...@chromium.org
█ yzshen@chromium.org
█ tibell@chromium.org
sa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Mojo C++ bindings: Reconsider Clone() and Equals() methods

Project Member Reported by yzshen@chromium.org, Jun 1 2016 
Clone() and Equals() of structs/unions/maps/arrays/etc. need to be reconsidered to work more consistently with custom types.

Example 1: currently Array::Clone() will call Clone() of its elements if the element type declares a member type MoveOnlyTypeForCPP03; otherwise it uses copy assignment of its elements. That doesn't make sense anymore after we introduce custom types.

Example 2: currently Array::Equals() will call Equals() of its elements if the element type is "standard" mojo struct/union/map/array type; otherwise it uses ==.

Comment 1 by yzshen@chromium.org, Jun 1 2016 

I considered the approach of having optional Clone()/Equals() functions in StructTraits<Foo, CustomFoo>, but that doesn't work. Because with Array<CustomFoo> we don't have the knowledge that CustomFoo corresponds to Foo. Actually, it is legitimate to map multiple mojom types to CustomFoo.

So my proposal is that Array<Foo>::Clone()/Equals() calls Foo::Clone()/Equals() if available, otherwise calls Foo's copy operator and == operator.

WDYT?

Comment 2 by roc...@chromium.org, Jun 1 2016 

SGTM

Comment 3 by yzshen@chromium.org, Jun 1 2016

Owner: yzshen@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 3 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14c4e1dca371579ceef264b8e3272b07c8bb92cb

commit 14c4e1dca371579ceef264b8e3272b07c8bb92cb
Author: yzshen <yzshen@chromium.org>
Date: Fri Jun 03 16:03:55 2016

Mojo C++ bindings: more consistent Clone() and Equals().

With this change, the Clone() and Equals() methods of structs/unions/maps/arrays call Clone() and Equals() of elements if available, otherwise fall back on copy and == operator of elements.

BUG= 616527 

Review-Url: https://codereview.chromium.org/2031823002
Cr-Commit-Position: refs/heads/master@{#397717}

[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/mojo_public.gyp
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/BUILD.gn
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/array.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/associated_interface_ptr_info.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/associated_interface_request.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/interface_request.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/lib/array_internal.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/lib/array_serialization.h
[delete] https://crrev.com/137654a60d37788e5206bd62fb074ae4af04b392/mojo/public/cpp/bindings/lib/map_internal.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/lib/template_util.h
[delete] https://crrev.com/137654a60d37788e5206bd62fb074ae4af04b392/mojo/public/cpp/bindings/lib/value_traits.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/map.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/tests/equals_unittest.cc
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/tests/struct_traits_unittest.cc
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/tests/struct_with_traits_impl.cc
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/tests/struct_with_traits_impl.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/bindings/wtf_array.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/cpp/system/handle.h
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/interfaces/bindings/tests/struct_with_traits.mojom
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/interfaces/bindings/tests/test_unions.mojom
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/BUILD.gn
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/bindings.gyp
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/module-internal.h.tmpl
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
[add] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_template_definition.tmpl
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_declaration.tmpl
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl
[add] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_template_definition.tmpl
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/generators/mojom_cpp_generator.py
[modify] https://crrev.com/14c4e1dca371579ceef264b8e3272b07c8bb92cb/mojo/public/tools/bindings/pylib/mojom/generate/module.py

Comment 5 by yzshen@chromium.org, Jun 3 2016

Status: Fixed (was: Started)
►

Sign in to add a comment