Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694843003:
Add field-initializing factory static methods to generated mojo unions. (Closed)
|
Created:
3 years, 10 months ago by Sam McNally Modified:
3 years, 8 months ago Reviewers:
yzshen1 CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd field-initializing factory static methods to generated mojo unions.
Review-Url: https://codereview.chromium.org/2694843003
Cr-Commit-Position: refs/heads/master@{#463167}
Committed: https://chromium.googlesource.com/chromium/src/+/7dfe5a09c6622a2079b5cc94cfc0cd565e1c0601
Patch Set 1 : #Patch Set 2 : rebase #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 80 (68 generated)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sammc@chromium.org changed reviewers: + yzshen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
Sorry for late reply. I missed the notification mail at the beginning. (Please feel free to ping me earlier next time if I don't respond in one working day.) I am thinking maybe it doesn't gain us much to add these constructors: - The first constructor (which doesn't explicitly specify the tag) is not always available. Besides, sometimes it maybe a little confusing to readers. - The second/third constructor uses something like FooUnion::TagType<FooUnion::Tag::F_BAR>() to specify the type, which doesn't seem convenient enough. - We use much fewer unions comparing with structs. Besides, for union initialization users only need to set a single field (while users may need to set multiple fields for structs without constructors.) WDYT? Thanks!
On 2017/03/06 16:57:50, yzshen1 wrote: > Sorry for late reply. I missed the notification mail at the beginning. (Please > feel free to ping me earlier next time if I don't respond in one working day.) > > I am thinking maybe it doesn't gain us much to add these constructors: > - The first constructor (which doesn't explicitly specify the tag) is not always > available. Besides, sometimes it maybe a little confusing to readers. > When is it not available? > > - The second/third constructor uses something like > FooUnion::TagType<FooUnion::Tag::F_BAR>() to specify the type, which doesn't > seem convenient enough. Perhaps, but they should only matter when a type is repeated in the union. > > - We use much fewer unions comparing with structs. Besides, for union > initialization users only need to set a single field (while users may need to > set multiple fields for structs without constructors.) The inconvenience of generated unions and perhaps https://crbug.com/510123 may be contributing to that. Responses that consist of a nullable value and a nullable error where exactly one should be non-null are not uncommon. The problem isn't the number of statements, it's that a statement is required to produce an initialized union. With a constructor, only an expression is necessary; that enables more succinct code. Compare auto result_ptr = FooResultUnion::New(); result_ptr->set_result(FooResult::New(...)); callback.Run(std::move(result_ptr); with callback.Run(FooResultUnion::New(FooResult::New(...))); > > WDYT? Thanks!
On 2017/03/07 03:13:11, Sam McNally wrote:
> On 2017/03/06 16:57:50, yzshen1 wrote:
> > Sorry for late reply. I missed the notification mail at the beginning.
(Please
> > feel free to ping me earlier next time if I don't respond in one working
day.)
> >
> > I am thinking maybe it doesn't gain us much to add these constructors:
> > - The first constructor (which doesn't explicitly specify the tag) is not
> always
> > available. Besides, sometimes it maybe a little confusing to readers.
> >
>
> When is it not available?
When the union has multiple fields that are of the same type, we don't generate
those "tag-less" constructors. (IIUC)
Even if the constructors are available, sometimes which one is used is not
obvious:
union Foo {
uint16 field1,
uint32 field2
};
If the code reads "Foo::New(2)", readers may wonder which field is set.
I think the basic issue is that fields are not identified by their data types,
but their names.
> >
> > - The second/third constructor uses something like
> > FooUnion::TagType<FooUnion::Tag::F_BAR>() to specify the type, which doesn't
> > seem convenient enough.
>
> Perhaps, but they should only matter when a type is repeated in the union.
And that is the thing that I am a little concerned about. :)
> >
> > - We use much fewer unions comparing with structs. Besides, for union
> > initialization users only need to set a single field (while users may need
to
> > set multiple fields for structs without constructors.)
>
> The inconvenience of generated unions and perhaps https://crbug.com/510123 may
> be contributing to that. Responses that consist of a nullable value and a
> nullable error where exactly one should be non-null are not uncommon. The
> problem isn't the number of statements, it's that a statement is required to
> produce an initialized union. With a constructor, only an expression is
> necessary; that enables more succinct code. Compare
>
> auto result_ptr = FooResultUnion::New();
> result_ptr->set_result(FooResult::New(...));
> callback.Run(std::move(result_ptr);
>
> with
>
> callback.Run(FooResultUnion::New(FooResult::New(...)));
I can see the benefit to have more succinct code. But clear code is more
important, even if it is a little verbose.
Does the following alternative make sense?
union Foo {
uint32 field1,
uint32 field2,
bool field3
};
// Generated constructors:
Foo(Tag tag, uint32_t value) {
CHECK(tag == Tag::FIELD1 || tag == Tag::FIELD2);
if (tag == Tag::FIELD1) {
...
} else if (tag == Tag::FIELD2) {
...
}
}
Foo(Tag tag, bool value) {
CHECK(tag == Tag::FIELD3);
...
}
That way users don't need to deal with the magical template Foo::TagType<>, and
it is always obvious. But the cost is compile-time type safety. :/
WDYT? Thanks!
>
> >
> > WDYT? Thanks!
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:180001) has been deleted
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/07 05:05:48, yzshen1 wrote:
> On 2017/03/07 03:13:11, Sam McNally wrote:
> > On 2017/03/06 16:57:50, yzshen1 wrote:
> > > Sorry for late reply. I missed the notification mail at the beginning.
> (Please
> > > feel free to ping me earlier next time if I don't respond in one working
> day.)
> > >
> > > I am thinking maybe it doesn't gain us much to add these constructors:
> > > - The first constructor (which doesn't explicitly specify the tag) is not
> > always
> > > available. Besides, sometimes it maybe a little confusing to readers.
> > >
> >
> > When is it not available?
>
> When the union has multiple fields that are of the same type, we don't
generate
> those "tag-less" constructors. (IIUC)
>
> Even if the constructors are available, sometimes which one is used is not
> obvious:
> union Foo {
> uint16 field1,
> uint32 field2
> };
>
> If the code reads "Foo::New(2)", readers may wonder which field is set.
>
> I think the basic issue is that fields are not identified by their data types,
> but their names.
>
> > >
> > > - The second/third constructor uses something like
> > > FooUnion::TagType<FooUnion::Tag::F_BAR>() to specify the type, which
doesn't
> > > seem convenient enough.
> >
> > Perhaps, but they should only matter when a type is repeated in the union.
>
> And that is the thing that I am a little concerned about. :)
>
> > >
> > > - We use much fewer unions comparing with structs. Besides, for union
> > > initialization users only need to set a single field (while users may need
> to
> > > set multiple fields for structs without constructors.)
> >
> > The inconvenience of generated unions and perhaps https://crbug.com/510123
may
> > be contributing to that. Responses that consist of a nullable value and a
> > nullable error where exactly one should be non-null are not uncommon. The
> > problem isn't the number of statements, it's that a statement is required to
> > produce an initialized union. With a constructor, only an expression is
> > necessary; that enables more succinct code. Compare
> >
> > auto result_ptr = FooResultUnion::New();
> > result_ptr->set_result(FooResult::New(...));
> > callback.Run(std::move(result_ptr);
> >
> > with
> >
> > callback.Run(FooResultUnion::New(FooResult::New(...)));
>
> I can see the benefit to have more succinct code. But clear code is more
> important, even if it is a little verbose.
> Does the following alternative make sense?
>
> union Foo {
> uint32 field1,
> uint32 field2,
> bool field3
> };
>
> // Generated constructors:
> Foo(Tag tag, uint32_t value) {
> CHECK(tag == Tag::FIELD1 || tag == Tag::FIELD2);
> if (tag == Tag::FIELD1) {
> ...
> } else if (tag == Tag::FIELD2) {
> ...
> }
> }
>
> Foo(Tag tag, bool value) {
> CHECK(tag == Tag::FIELD3);
> ...
> }
>
> That way users don't need to deal with the magical template Foo::TagType<>,
and
> it is always obvious. But the cost is compile-time type safety. :/
>
> WDYT? Thanks!
>
>
I've added nicer tag type values so that the calling side is
Foo::New(Foo::field1, value);
and removed the single parameter overloads for primitive types.
>
> >
> > >
> > > WDYT? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2694843003/diff/200001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_declaration.tmpl (right): https://codereview.chromium.org/2694843003/diff/200001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_declaration.tmpl:34: explicit {{union.name}}(Tag tag); With the current constructors, users would do: FooUnion(FooUnion::Tag::BAR); FooUnion(FooUnion::bar, bar_value); It seems unfortunate to have Tag and TagTypes_. We should avoid having two ways of specifying tags. I would rather we use Tag values consistently, although we lose some compile-time type safety. More thoughts on naming for discussion: actually the current field getter FooUnion::get_bar() is not very consistent with our style guide, we should have used FooUnion::bar() as the getter name. WDYT? https://codereview.chromium.org/2694843003/diff/200001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_declaration.tmpl:40: explicit {{union.name}}({{field.kind|cpp_wrapper_param_type}} in) I still think we should avoid this distinct-type-to-identify-field overload. https://codereview.chromium.org/2694843003/diff/200001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_declaration.tmpl:44: {{union.name}}(TagTypes_::{{field.name|under_to_camel}}); There is already a similar one on line 34. And it is less consistent: a constructor for object fields but not primitive type fields.
Description was changed from ========== Add initializing constructors to generated mojo unions. ========== to ========== Add field-initializing factory static methods to generated mojo unions. ==========
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Patchset #4 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:240001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've changed this to add factory static methods to generated unions instead of constructors. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM Thanks! https://codereview.chromium.org/2694843003/diff/260001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl (right): https://codereview.chromium.org/2694843003/diff/260001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:3: {%- if default_field.kind|is_string_kind or default_field.kind|is_object_kind or nit: is_object_kind includes is_string_kind, so it should be okey to remove the first part.
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2694843003/diff/260001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl (right): https://codereview.chromium.org/2694843003/diff/260001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:3: {%- if default_field.kind|is_string_kind or default_field.kind|is_object_kind or On 2017/04/09 23:32:31, yzshen1 wrote: > nit: is_object_kind includes is_string_kind, so it should be okey to remove the > first part. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2694843003/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491792450834160,
"parent_rev": "7e01649ec8b61ba9f5e934bf036eb996530e7d7b", "commit_rev":
"7dfe5a09c6622a2079b5cc94cfc0cd565e1c0601"}
Message was sent while issue was closed.
Description was changed from ========== Add field-initializing factory static methods to generated mojo unions. ========== to ========== Add field-initializing factory static methods to generated mojo unions. Review-Url: https://codereview.chromium.org/2694843003 Cr-Commit-Position: refs/heads/master@{#463167} Committed: https://chromium.googlesource.com/chromium/src/+/7dfe5a09c6622a2079b5cc94cfc0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001) as https://chromium.googlesource.com/chromium/src/+/7dfe5a09c6622a2079b5cc94cfc0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
