> 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
>
>
