Issue 622593002: mojo: Allow circular dependencies between structs

Issue 622593002: mojo: Allow circular dependencies between structs (Closed)

Created:
6 years, 2 months ago by qsr

Modified:
6 years, 2 months ago

Reviewers:
darin (slow to review), viettrungluu

CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

mojo: Allow circular dependencies between structs This fix the C++ and python generation to allow circular dependencies between structs. R=viettrungluu@chromium.org Committed: https://crrev.com/1e0b95af8d9bbcfbf2ddf68a9e8cebb7e1d79a2f Cr-Commit-Position: refs/heads/master@{#299079}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Follow review #

Patch Set 3 : Use EnableIf #

Patch Set 4 : Rebase #

Total comments: 2

Patch Set 5 : Add comments. #

Patch Set 6 : Add missing space. #

Patch Set 7 : Rebasing #

Patch Set 8 : Rebase #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+89 lines, -14 lines)			Patch
M	mojo/public/cpp/bindings/lib/array_serialization.h	View	1 2 3 4 5 6 7	4 chunks	+18 lines, -8 lines	0 comments	Download
M	mojo/public/cpp/bindings/lib/bindings_internal.h	View	1 2 3 4 5 6	2 chunks	+9 lines, -0 lines	0 comments	Download
M	mojo/public/cpp/bindings/lib/template_util.h	View	1 2 3 4 5 6	2 chunks	+12 lines, -0 lines	0 comments	Download
M	mojo/public/cpp/bindings/struct_ptr.h	View	1 2 3 4 5 6	3 chunks	+1 line, -2 lines	0 comments	Download
M	mojo/public/cpp/bindings/tests/serialization_warning_unittest.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	mojo/public/interfaces/bindings/tests/regression_tests.mojom	View	1 2	1 chunk	+19 lines, -0 lines	0 comments	Download
M	mojo/public/python/mojo/bindings/descriptor.py	View	1 2 3	1 chunk	+9 lines, -2 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/cpp_templates/module-internal.h.tmpl	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -0 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/mojom_python_generator.py	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	mojo/python/tests/bindings_structs_unittest.py	View	1 2 3	2 chunks	+9 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

viettrungluu

https://codereview.chromium.org/622593002/diff/1/mojo/public/cpp/bindings/lib/array_serialization.h File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/622593002/diff/1/mojo/public/cpp/bindings/lib/array_serialization.h#newcode123 mojo/public/cpp/bindings/lib/array_serialization.h:123: typename internal::WrapperTraits<S>::PlainDataType*, internal::WrapperTraits<S>::PlainDataType* is just internal::WrapperTraits<S>::DataType. I'm guessing that ...

6 years, 2 months ago (2014-10-01 17:18:23 UTC) #2

qsr

Patch Set 2 is the same change, with your comments taken into account Patch Set ...

6 years, 2 months ago (2014-10-06 08:20:26 UTC) #3

Patch Set 2 is the same change, with your comments taken into account

Patch Set 3 is using EnableIf.

 Tell me which one you like better. EnableIf allow not to add another type to
WrapperTrait, but the template declaration of ArraySerializer if really close to
being unreadable.

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
mojo/public/cpp/bindings/lib/array_serialization.h:123: typename
internal::WrapperTraits<S>::PlainDataType*,
On 2014/10/01 17:18:23, viettrungluu wrote:
> internal::WrapperTraits<S>::PlainDataType* is just
> internal::WrapperTraits<S>::DataType.
> 
> I'm guessing that you use this to ensure that this template is only applied to
> structs?
> 
> I wonder if using a suitable enable_if wouldn't be cleaner.

 enable_if, as well as remove_pointer are in type_traits

> (Note also that you can get a T from a typename which is a T*, via suitable
> template magic; the C++11 <type_traits> has remove_pointer which we can't use
> yet, but it's easy enough to implement yourself. Similarly, you can also tell
if
> a given typename is a pointer type, if you need to do so to use with some
> enable_if or similar.)

 Yes, I need this to prevent this one to be possible for handles. Uploaded a
change using EnableIf and RemovePointer instead.

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
mojo/public/cpp/bindings/lib/array_serialization.h:125: typedef typename
internal::WrapperTraits<S>::PlainDataType F;
On 2014/10/01 17:18:23, viettrungluu wrote:
> nit: A more descriptive name (even "S_Data") than "F" would be nice.

Done.

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
mojo/public/cpp/bindings/lib/bindings_internal.h:82: typedef typename S::Data_
PlainDataType;
On 2014/10/01 17:18:23, viettrungluu wrote:
> I'm not a huge fan of the name "PlainDataType", but I can't think of something
> better off the top of my head.

 Unfortunately, I don't have anything that really fit, unless we want to be very
long.

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
File mojo/public/interfaces/bindings/tests/regression_tests.mojom (right):

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
mojo/public/interfaces/bindings/tests/regression_tests.mojom:38: Vertice? v;
On 2014/10/01 17:18:23, viettrungluu wrote:
> nit: Vertice -> Vertex

Done.

https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
mojo/public/interfaces/bindings/tests/regression_tests.mojom:41: struct Vertice
{
On 2014/10/01 17:18:23, viettrungluu wrote:
> "

Done.

viettrungluu

On 2014/10/06 08:20:26, qsr wrote: > Patch Set 2 is the same change, with your ...

6 years, 2 months ago (2014-10-06 23:08:22 UTC) #4

On 2014/10/06 08:20:26, qsr wrote:
> Patch Set 2 is the same change, with your comments taken into account
> 
> Patch Set 3 is using EnableIf.
> 
>  Tell me which one you like better. EnableIf allow not to add another type to
> WrapperTrait, but the template declaration of ArraySerializer if really close
to
> being unreadable.

I kind of like the explicitness of the EnableIf version, but things do get
pretty ugly.

Yeah, maybe some comments would help. ;)

> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
> mojo/public/cpp/bindings/lib/array_serialization.h:123: typename
> internal::WrapperTraits<S>::PlainDataType*,
> On 2014/10/01 17:18:23, viettrungluu wrote:
> > internal::WrapperTraits<S>::PlainDataType* is just
> > internal::WrapperTraits<S>::DataType.
> > 
> > I'm guessing that you use this to ensure that this template is only applied
to
> > structs?
> > 
> > I wonder if using a suitable enable_if wouldn't be cleaner.
> 
>  enable_if, as well as remove_pointer are in type_traits
> 
> > (Note also that you can get a T from a typename which is a T*, via suitable
> > template magic; the C++11 <type_traits> has remove_pointer which we can't
use
> > yet, but it's easy enough to implement yourself. Similarly, you can also
tell
> if
> > a given typename is a pointer type, if you need to do so to use with some
> > enable_if or similar.)
> 
>  Yes, I need this to prevent this one to be possible for handles. Uploaded a
> change using EnableIf and RemovePointer instead.
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
> mojo/public/cpp/bindings/lib/array_serialization.h:125: typedef typename
> internal::WrapperTraits<S>::PlainDataType F;
> On 2014/10/01 17:18:23, viettrungluu wrote:
> > nit: A more descriptive name (even "S_Data") than "F" would be nice.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
> File mojo/public/cpp/bindings/lib/bindings_internal.h (right):
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/cpp/bindi...
> mojo/public/cpp/bindings/lib/bindings_internal.h:82: typedef typename S::Data_
> PlainDataType;
> On 2014/10/01 17:18:23, viettrungluu wrote:
> > I'm not a huge fan of the name "PlainDataType", but I can't think of
something
> > better off the top of my head.
> 
>  Unfortunately, I don't have anything that really fit, unless we want to be
very
> long.
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
> File mojo/public/interfaces/bindings/tests/regression_tests.mojom (right):
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
> mojo/public/interfaces/bindings/tests/regression_tests.mojom:38: Vertice? v;
> On 2014/10/01 17:18:23, viettrungluu wrote:
> > nit: Vertice -> Vertex
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/622593002/diff/1/mojo/public/interface...
> mojo/public/interfaces/bindings/tests/regression_tests.mojom:41: struct
Vertice
> {
> On 2014/10/01 17:18:23, viettrungluu wrote:
> > "
> 
> Done.

viettrungluu

+darin, in case he has a better idea https://codereview.chromium.org/622593002/diff/50001/mojo/public/cpp/bindings/lib/template_util.h File mojo/public/cpp/bindings/lib/template_util.h (right): https://codereview.chromium.org/622593002/diff/50001/mojo/public/cpp/bindings/lib/template_util.h#newcode89 mojo/public/cpp/bindings/lib/template_util.h:89: template<class ...

6 years, 2 months ago (2014-10-06 23:08:40 UTC) #6

qsr

Comments added. Darin, WDYT? https://codereview.chromium.org/622593002/diff/50001/mojo/public/cpp/bindings/lib/template_util.h File mojo/public/cpp/bindings/lib/template_util.h (right): https://codereview.chromium.org/622593002/diff/50001/mojo/public/cpp/bindings/lib/template_util.h#newcode89 mojo/public/cpp/bindings/lib/template_util.h:89: template<class T> struct RemovePointer{}; On ...

6 years, 2 months ago (2014-10-07 08:56:12 UTC) #7

qsr

Trung, given that we do not have any answer from Darin, how do you suggest ...

6 years, 2 months ago (2014-10-09 08:51:15 UTC) #8

viettrungluu

On 2014/10/09 08:51:15, qsr wrote: > Trung, given that we do not have any answer ...

6 years, 2 months ago (2014-10-09 20:29:03 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/622593002/140001

6 years, 2 months ago (2014-10-10 07:44:27 UTC) #11

commit-bot: I haz the power

Patchset 8 (id:??) landed as https://crrev.com/1e0b95af8d9bbcfbf2ddf68a9e8cebb7e1d79a2f Cr-Commit-Position: refs/heads/master@{#299079}

6 years, 2 months ago (2014-10-10 09:00:20 UTC) #13

Message was sent while issue was closed.

Seems like a fine solution to me. LGTM

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages