You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2015/08/27 05:38:48 UTC
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 3:38 a.m.)
Review request for mesos and Michael Park.
Changes
-------
Rebased.
Summary (updated)
-----------------
Introduced conversion of JSON arrays to repeated protobufs.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review96709
-----------------------------------------------------------
Ship it!
Ship It!
Note: This would be useful for maintenance primitives too!
- Joseph Wu
On Aug. 26, 2015, 8:38 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 8:38 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 31, 2015, 11:20 a.m., Michael Park wrote:
> > What do you think of the following API?
> >
> > ```
> > JSON::Value value = ...;
> > auto message = protobuf::parse<T>(value); // message has type Try<T>.
> > auto repeated = protobuf::parse<google::protobuf::RepeatedPtrField<T>>(value); // repeated has type google::protobuf::RepeatedPtrField<T>.
> > ```
> >
> > This makes it so that `parse<T>` always returns `Try<T>`, which I think enables the use of `auto` for the result under our style guide.
> > I think this is a simpler pattern to remember than `parse<T>` -> `Try<T>` and `parseRepeated<T>` -> `Try<google::protobuf::RepeatedPtrField<T>>`.
> >
> > It would take `JSON::Value` rather than `JSON::Array` but check that it is indeed an instance of `JSON::Array`,
> > the same way the existing version takes `JSON::Value` and checks that it's an instance of `JSON::Object`.
> >
> > It also makes the API symmetric with `read()`. We simply do `read<T>` or `read<google::protobuf::RepeatedPtrField<T>>` and it does the right thing.
>
> Michael Park wrote:
> __NOTE:__ `parseRepeated` is actually @joseph's suggestion in the subsequent patch.
>
> In this patch, `parse<T>(value)` returns `Try<T>` and `parse<T>(array)` returns `Try<google::protobuf::RepeatedPtrField<T>>`,
> which I think is just as hard if not harder to intuitively deduce.
>
> What do you think?
I think that would be good. It seems more verbose, but also a lot more explicit.
(But I can imagine that it might not fit in 80 characters in some places...)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97110
-----------------------------------------------------------
On Aug. 26, 2015, 8:38 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 8:38 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 31, 2015, 6:20 p.m., Michael Park wrote:
> > What do you think of the following API?
> >
> > ```
> > JSON::Value value = ...;
> > auto message = protobuf::parse<T>(value); // message has type Try<T>.
> > auto repeated = protobuf::parse<google::protobuf::RepeatedPtrField<T>>(value); // repeated has type google::protobuf::RepeatedPtrField<T>.
> > ```
> >
> > This makes it so that `parse<T>` always returns `Try<T>`, which I think enables the use of `auto` for the result under our style guide.
> > I think this is a simpler pattern to remember than `parse<T>` -> `Try<T>` and `parseRepeated<T>` -> `Try<google::protobuf::RepeatedPtrField<T>>`.
> >
> > It would take `JSON::Value` rather than `JSON::Array` but check that it is indeed an instance of `JSON::Array`,
> > the same way the existing version takes `JSON::Value` and checks that it's an instance of `JSON::Object`.
> >
> > It also makes the API symmetric with `read()`. We simply do `read<T>` or `read<google::protobuf::RepeatedPtrField<T>>` and it does the right thing.
>
> Michael Park wrote:
> __NOTE:__ `parseRepeated` is actually @joseph's suggestion in the subsequent patch.
>
> In this patch, `parse<T>(value)` returns `Try<T>` and `parse<T>(array)` returns `Try<google::protobuf::RepeatedPtrField<T>>`,
> which I think is just as hard if not harder to intuitively deduce.
>
> What do you think?
>
> Joseph Wu wrote:
> I think that would be good. It seems more verbose, but also a lot more explicit.
>
> (But I can imagine that it might not fit in 80 characters in some places...)
> (But I can imagine that it might not fit in 80 characters in some places...)
Yeah, but I think that's fine. It's the case with most uses of `google::protobuf::RepeatedPtrField` across the codebase currently.
We can also do `using google::protobuf::RepeatedPtrField;` in places where possible.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97110
-----------------------------------------------------------
On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 3:38 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 31, 2015, 6:20 p.m., Michael Park wrote:
> > What do you think of the following API?
> >
> > ```
> > JSON::Value value = ...;
> > auto message = protobuf::parse<T>(value); // message has type Try<T>.
> > auto repeated = protobuf::parse<google::protobuf::RepeatedPtrField<T>>(value); // repeated has type google::protobuf::RepeatedPtrField<T>.
> > ```
> >
> > This makes it so that `parse<T>` always returns `Try<T>`, which I think enables the use of `auto` for the result under our style guide.
> > I think this is a simpler pattern to remember than `parse<T>` -> `Try<T>` and `parseRepeated<T>` -> `Try<google::protobuf::RepeatedPtrField<T>>`.
> >
> > It would take `JSON::Value` rather than `JSON::Array` but check that it is indeed an instance of `JSON::Array`,
> > the same way the existing version takes `JSON::Value` and checks that it's an instance of `JSON::Object`.
> >
> > It also makes the API symmetric with `read()`. We simply do `read<T>` or `read<google::protobuf::RepeatedPtrField<T>>` and it does the right thing.
__NOTE:__ `parseRepeated` is actually @joseph's suggestion in the subsequent patch.
In this patch, `parse<T>(value)` returns `Try<T>` and `parse<T>(array)` returns `Try<google::protobuf::RepeatedPtrField<T>>`,
which I think is just as hard if not harder to intuitively deduce.
What do you think?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97110
-----------------------------------------------------------
On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 3:38 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 31, 2015, 6:20 p.m., Michael Park wrote:
> > What do you think of the following API?
> >
> > ```
> > JSON::Value value = ...;
> > auto message = protobuf::parse<T>(value); // message has type Try<T>.
> > auto repeated = protobuf::parse<google::protobuf::RepeatedPtrField<T>>(value); // repeated has type google::protobuf::RepeatedPtrField<T>.
> > ```
> >
> > This makes it so that `parse<T>` always returns `Try<T>`, which I think enables the use of `auto` for the result under our style guide.
> > I think this is a simpler pattern to remember than `parse<T>` -> `Try<T>` and `parseRepeated<T>` -> `Try<google::protobuf::RepeatedPtrField<T>>`.
> >
> > It would take `JSON::Value` rather than `JSON::Array` but check that it is indeed an instance of `JSON::Array`,
> > the same way the existing version takes `JSON::Value` and checks that it's an instance of `JSON::Object`.
> >
> > It also makes the API symmetric with `read()`. We simply do `read<T>` or `read<google::protobuf::RepeatedPtrField<T>>` and it does the right thing.
>
> Michael Park wrote:
> __NOTE:__ `parseRepeated` is actually @joseph's suggestion in the subsequent patch.
>
> In this patch, `parse<T>(value)` returns `Try<T>` and `parse<T>(array)` returns `Try<google::protobuf::RepeatedPtrField<T>>`,
> which I think is just as hard if not harder to intuitively deduce.
>
> What do you think?
>
> Joseph Wu wrote:
> I think that would be good. It seems more verbose, but also a lot more explicit.
>
> (But I can imagine that it might not fit in 80 characters in some places...)
>
> Michael Park wrote:
> > (But I can imagine that it might not fit in 80 characters in some places...)
>
> Yeah, but I think that's fine. It's the case with most uses of `google::protobuf::RepeatedPtrField` across the codebase currently.
> We can also do `using google::protobuf::RepeatedPtrField;` in places where possible.
I think it's a great idea, I'll go for the partial specialization trick.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97110
-----------------------------------------------------------
On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 3:38 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97110
-----------------------------------------------------------
What do you think of the following API?
```
JSON::Value value = ...;
auto message = protobuf::parse<T>(value); // message has type Try<T>.
auto repeated = protobuf::parse<google::protobuf::RepeatedPtrField<T>>(value); // repeated has type google::protobuf::RepeatedPtrField<T>.
```
This makes it so that `parse<T>` always returns `Try<T>`, which I think enables the use of `auto` for the result under our style guide.
I think this is a simpler pattern to remember than `parse<T>` -> `Try<T>` and `parseRepeated<T>` -> `Try<google::protobuf::RepeatedPtrField<T>>`.
It would take `JSON::Value` rather than `JSON::Array` but check that it is indeed an instance of `JSON::Array`,
the same way the existing version takes `JSON::Value` and checks that it's an instance of `JSON::Object`.
It also makes the API symmetric with `read()`. We simply do `read<T>` or `read<google::protobuf::RepeatedPtrField<T>>` and it does the right thing.
- Michael Park
On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 3:38 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 10, 2015, 8:44 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 606
> > <https://reviews.apache.org/r/37826/diff/6/?file=1066088#file1066088line606>
> >
> > Would `_value` be better than `v` here? Maybe `elem`...?
I don't like `_*` variables and prefer to avoid them where possible (not dictated by our style guide). `elem` sounds good.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review98370
-----------------------------------------------------------
On Sept. 9, 2015, 2:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 2:46 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 10, 2015, 8:44 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 626-627
> > <https://reviews.apache.org/r/37826/diff/6/?file=1066088#file1066088line626>
> >
> > > overload will not work because ...
> >
> > As I mentioned in my comment in reply to Joseph's question, the overloaded functions approach with the use of `enable_if` would work but not as clean.
I didn't mean to `enable_if` here, but rather a common function overload. I'll update the comment for clarity.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review98370
-----------------------------------------------------------
On Sept. 9, 2015, 2:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 2:46 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review98370
-----------------------------------------------------------
Ship it!
Looks good!
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 602)
<https://reviews.apache.org/r/37826/#comment154831>
Would `_value` be better than `v` here? Maybe `elem`...?
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 622 - 623)
<https://reviews.apache.org/r/37826/#comment154828>
> overload will not work because ...
As I mentioned in my comment in reply to Joseph's question, the overloaded functions approach with the use of `enable_if` would work but not as clean.
- Michael Park
On Sept. 9, 2015, 2:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 2:46 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Gilbert Song <gi...@mesoshere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review99296
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Sept. 10, 2015, 6:31 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2015, 6:31 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Sept. 10, 2015, 6:31 p.m.)
Review request for mesos, Joseph Wu and Michael Park.
Changes
-------
MPark's comments.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 2:46 p.m.)
Review request for mesos, Joseph Wu and Michael Park.
Changes
-------
Rebased.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Sept. 7, 2015, 9:12 a.m.)
Review request for mesos, Joseph Wu and Michael Park.
Changes
-------
Michael's comments.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 3, 2015, 11:32 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 594
> > <https://reviews.apache.org/r/37826/diff/4/?file=1063276#file1063276line594>
> >
> > Do we want to support the following?
> >
> > ```cpp
> > using google::protobuf::RepeatedPtrField;
> >
> > auto parse = protobuf::parse<RepeatedPtrField<RepeatedPtrField<T>>>(array_of_array);
> > ```
> >
> > I think we might want to disallow it since it seems that protobuf definitions cannot express "array of array" (maybe there actually is a way to do that?). If we do want to disallow it, we should include the following check at the top.
> >
> > ```cpp
> > { google::protobuf::Message* message = (T*) NULL; (void) message; }
> > ```
A brilliant suggestion, Michael!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97693
-----------------------------------------------------------
On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97693
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 592)
<https://reviews.apache.org/r/37826/#comment153702>
Do we want to support the following?
```cpp
using google::protobuf::RepeatedPtrField;
auto parse = protobuf::parse<RepeatedPtrField<RepeatedPtrField<T>>>(array_of_array);
```
I think we might want to disallow it since it seems that protobuf definitions cannot express "array of array" (maybe there actually is a way to do that?). If we do want to disallow it, we should include the following check at the top.
```cpp
{ google::protobuf::Message* message = (T*) NULL; (void) message; }
```
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 601)
<https://reviews.apache.org/r/37826/#comment153704>
`s/proagate/propagate/`
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 604)
<https://reviews.apache.org/r/37826/#comment153703>
Remove this newline.
- Michael Park
On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 3, 2015, 8:37 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 593
> > <https://reviews.apache.org/r/37826/diff/4/?file=1063276#file1063276line593>
> >
> > The `Parse<T>` above uses `value`, rather than `json`. Can we consolidate to one? I think `value` would be more fitting, since we name `JSON::Object` as `object`, `JSON::Array` as `array`, etc.
I have a nested `foreach` loop where I use `value` already. I'll rename that one, if you prefer to keep `value` in the signature.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97674
-----------------------------------------------------------
On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97674
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 559)
<https://reviews.apache.org/r/37826/#comment153688>
Remove this newline.
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 567)
<https://reviews.apache.org/r/37826/#comment153689>
Remove this newline.
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 591)
<https://reviews.apache.org/r/37826/#comment153690>
The `Parse<T>` above uses `value`, rather than `json`. Can we consolidate to one? I think `value` would be more fitting, since we name `JSON::Object` as `object`, `JSON::Array` as `array`, etc.
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 627)
<https://reviews.apache.org/r/37826/#comment153691>
`s/json/value/` as suggested above?
- Michael Park
On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Sept. 3, 2015, 1:32 p.m.)
Review request for mesos, Joseph Wu and Michael Park.
Changes
-------
Expanded a comment.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:46 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
>
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
Yes, I think a comment would be good regardless of how it ends up.
What about using some C++11 type traits? (But I'm not sure if this will work.)
Something like:
```
// Specialization for non-repeated fields.
template <typename T>
Try<T> parse(const JSON::Value& value,
typename std::enable_if<std::is_base_of<T, google::protobuf::Message>, int>::type = 0) {...}
```
Note: This file uses some similar Boost type traits further up.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 1, 2015, 7:20 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 7:20 a.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
>
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
>
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
>
> What about using some C++11 type traits? (But I'm not sure if this will work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template <typename T>
> Try<T> parse(const JSON::Value& value,
> typename std::enable_if<std::is_base_of<T, google::protobuf::Message>, int>::type = 0) {...}
> ```
>
> Note: This file uses some similar Boost type traits further up.
>
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate compile-time checks, and not compile-time dispatch. If we want to keep the same function signature, we should rely on partial specialization. However, we can still add these checks into `Parse<>::operator()` methods.
Which boils down to replacing
```
{ google::protobuf::Message* message = (T*) NULL; (void) message; }
```
with
```
static_assert(std::is_base_of<google::protobuf::Message, T>::value,
"T must be a protobuf message");
```
Does it make sense?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 2:20 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Michael Park <mc...@gmail.com>.
> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
>
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
>
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
>
> What about using some C++11 type traits? (But I'm not sure if this will work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template <typename T>
> Try<T> parse(const JSON::Value& value,
> typename std::enable_if<std::is_base_of<T, google::protobuf::Message>, int>::type = 0) {...}
> ```
>
> Note: This file uses some similar Boost type traits further up.
>
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate compile-time checks, and not compile-time dispatch. If we want to keep the same function signature, we should rely on partial specialization. However, we can still add these checks into `Parse<>::operator()` methods.
>
> Alexander Rukletsov wrote:
> Which boils down to replacing
> ```
> { google::protobuf::Message* message = (T*) NULL; (void) message; }
> ```
> with
> ```
> static_assert(std::is_base_of<google::protobuf::Message, T>::value,
> "T must be a protobuf message");
> ```
> Does it make sense?
>
> Joseph Wu wrote:
> That part makes sense.
>
> And just to make sure, you can't use the same pattern found here? (Which looks a lot like compile-time dispatch.)
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L165-L177
We could use the overloaded functions approach using `enable_if`. But it's not as clean, and I'll explain what I mean.
First thing to note is that the types are passed explicitly, rather than being deduced from parameters.
They would be deduced and can be handled by overload resolution if, for example, we opted for a design where we pass the out-parameter.
```cpp
template <typename T>
bool parse(const JSON::Value& value, T* out); // (1)
template <typename T>
bool parse(const JSON::Value& value, google::protobuf::RepeatedPtrField<T>* out); // (2)
google::protobuf::Resource resource;
bool a = parse(value, resource) // invokes (1)
// Check 'a' to determine success.
google::protobuf::RepeatedPtrField<Resource> resources;
bool b = parse(value, resources); // invokes (2)
// Check 'b' to determine success.
```
We dispatch according to the overload resolution rules based on how the second paramter gets deduced and matches.
With overloaded functions using `enable_if`, the conditions must be mutually exclusive,
otherwise the intersections trigger ambiguous overload errors.
The following is what we would need to make it work for our current case.
First we need a helper to determine whether a given `T` is an instance of `google::protobuf::RepeatedPtrField` or not.
This is similar to `std::is_integral`, and other type traits helpers. We need this to use as our `enable_if` condition.
```cpp
// Use type deduction and overload resolution rules to help determine whether `T` is an instance of
// `google::protobuf::RepeatedPtrField<U>` for some `U`.
template <typename T>
struct is_repeated
{
template <typename U>
static std::false_type impl(U &&);
template <typename U>
static std::true_type impl(google::protobuf::RepeatedPtrField<U> &&);
static const bool value = decltype(impl(std::declval<T>()))::value;
}
```
We can then use it to mutually exclusively define our `enable_if` conditions:
```cpp
// T is not an instance of google::protobuf::RepeatedPtrField<U> for any U.
template <typename T>
std::enable_if_t<!is_repeated<T>::value, Try<T>> parse(const JSON::Value& value);
// T is google::protobuf::RepeatedPtrField<U> for some U.
template <typename T>
std::enable_if_t<is_repeated<T>::value, Try<T>> parse(const JSON::Value& value);
```
Now, to the not-so-clean parts. The first one is that we don't immediately have access to the inner type.
That is, inside the body of:
```cpp
// T is google::protobuf::RepeatedPtrField<U> for some U.
template <typename T>
std::enable_if_t<is_repeated<T>::value, Try<T>> parse(const JSON::Value& value);
```
We know that `T` is a `google::protobuf::RepeatedPtrField<U>` for some `U`, but there's not a `U` for us to use immediately.
We'll have to introduce another type trait like `get_inner<T>` which returns the inner type `U` for us.
The other, and more important is that what we express is fundamentally different between partial specializations and
overloaded functions with `enable_if`.
Partial specialization gives us the closest thing to pattern matching in C++.
We specify the patterns and dispatch to the best match available.
We can say `general`, `A`, `B`, ..., where `A` and `B` doesn't even need to be mutually exclusive, just one needs to be a __better__ match.
In general, when we're matching on patterns I believe partial specialization is a cleaner technique.
Overloaded functions with `enable_if` on the other hand match based on mutually exclusive boolean conditions.
This means that to express the meaning of `general`, `A`, `B` (let's simplify by assuming `A` and `B` are mutually exclusive),
we would still have to write the conditions as: `A`, `B`, `!A && !B`.
In general, this technique is useful when we want to match on categories of types (e.g. `std::is_integral`) or
when you want to conditionally take one of the overloads out of overload resolution.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:46 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
>
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
>
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
>
> What about using some C++11 type traits? (But I'm not sure if this will work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template <typename T>
> Try<T> parse(const JSON::Value& value,
> typename std::enable_if<std::is_base_of<T, google::protobuf::Message>, int>::type = 0) {...}
> ```
>
> Note: This file uses some similar Boost type traits further up.
>
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate compile-time checks, and not compile-time dispatch. If we want to keep the same function signature, we should rely on partial specialization. However, we can still add these checks into `Parse<>::operator()` methods.
>
> Alexander Rukletsov wrote:
> Which boils down to replacing
> ```
> { google::protobuf::Message* message = (T*) NULL; (void) message; }
> ```
> with
> ```
> static_assert(std::is_base_of<google::protobuf::Message, T>::value,
> "T must be a protobuf message");
> ```
> Does it make sense?
That part makes sense.
And just to make sure, you can't use the same pattern found here? (Which looks a lot like compile-time dispatch.)
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L165-L177
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 3, 2015, 6:32 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 6:32 a.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 2:20 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
>
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
>
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
>
> What about using some C++11 type traits? (But I'm not sure if this will work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template <typename T>
> Try<T> parse(const JSON::Value& value,
> typename std::enable_if<std::is_base_of<T, google::protobuf::Message>, int>::type = 0) {...}
> ```
>
> Note: This file uses some similar Boost type traits further up.
I'm not sure how we can use type traits here. IIUC type traits facilitate compile-time checks, and not compile-time dispatch. If we want to keep the same function signature, we should rely on partial specialization. However, we can still add these checks into `Parse<>::operator()` methods.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 2:20 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > <https://reviews.apache.org/r/37826/diff/3/?file=1061137#file1061137line553>
> >
> > Can you explain why this is a struct rather than a function?
>
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and overload won't work since we take the same argument. Do you think a comment should be expanded?
http://www.gotw.ca/publications/mill17.htm
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 2:20 p.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/#review97302
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 551 - 553)
<https://reviews.apache.org/r/37826/#comment153139>
Can you explain why this is a struct rather than a function?
- Joseph Wu
On Sept. 1, 2015, 7:20 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 7:20 a.m.)
>
>
> Review request for mesos, Joseph Wu and Michael Park.
>
>
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
>
> Diff: https://reviews.apache.org/r/37826/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 37826: Introduced conversion of JSON arrays to
repeated protobufs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37826/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 2:20 p.m.)
Review request for mesos, Joseph Wu and Michael Park.
Changes
-------
Implemented MPark's suggestion.
Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
Diff: https://reviews.apache.org/r/37826/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov