mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.
Date Thu, 03 Sep 2015 21:26:01 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message