You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2016/01/11 10:25:29 UTC

Re: Review Request 41882: Constrained types used in Flags instantiation.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/
-----------------------------------------------------------

(Updated Jan. 11, 2016, 10:25 a.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
-------

Translated summary and description to standard English.


Summary (updated)
-----------------

Constrained types used in Flags instantiation.


Bugs: MESOS-4278
    https://issues.apache.org/jira/browse/MESOS-4278


Repository: mesos


Description (updated)
-------

Constrained types used in Flags instantiation.


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 

Diff: https://reviews.apache.org/r/41882/diff/


Testing
-------


Thanks,

Benjamin Bannier

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 11, 2016, 1:26 p.m., Alexander Rojas wrote:
> > Why not trying this:
> > 
> > ```c++
> > template <typename ...FlagsTypes>
> > class Flags : public virtual FlagsTypes...
> > {
> >   static_assert(
> >       std::is_same<
> >           std::tuple<typename std::is_base_of<FlagsBase, FlagsTypes>::type...>,
> >           std::tuple<typename std::is_object<FlagsTypes>::type...>>::value,
> >       "Can only instantiate Flags with FlagsBase types.");
> > };
> > 
> > 
> > template <>
> > class Flags<> : public virtual FlagsBase
> > {};
> > ```
> > 
> > It seems more concise, it is easy to read and allows an arbitrary number of parameters. On the down side, you won't know exactly which type broke the condition.

That looks like a good idea! I personally find it pretty self-documenting, and it removes the dummy types for default template args; I'll check with my shepherd tomorrow to gauge what kind of additional documentation this might need for mesos.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review113739
-----------------------------------------------------------


On Jan. 11, 2016, 10:25 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 10:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Constrained types used in Flags instantiation.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Alexander Rojas <al...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review113739
-----------------------------------------------------------


Why not trying this:

```c++
template <typename ...FlagsTypes>
class Flags : public virtual FlagsTypes...
{
  static_assert(
      std::is_same<
          std::tuple<typename std::is_base_of<FlagsBase, FlagsTypes>::type...>,
          std::tuple<typename std::is_object<FlagsTypes>::type...>>::value,
      "Can only instantiate Flags with FlagsBase types.");
};


template <>
class Flags<> : public virtual FlagsBase
{};
```

It seems more concise, it is easy to read and allows an arbitrary number of parameters. On the down side, you won't know exactly which type broke the condition.

- Alexander Rojas


On Jan. 11, 2016, 10:25 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 10:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Constrained types used in Flags instantiation.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Joerg Schad <jo...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review113708
-----------------------------------------------------------

Ship it!


Ship It!

- Joerg Schad


On Jan. 11, 2016, 9:25 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 9:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Constrained types used in Flags instantiation.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Alexander Rojas <al...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review114177
-----------------------------------------------------------

Ship it!


Ship It!

- Alexander Rojas


On Jan. 13, 2016, 11:43 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2016, 11:43 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> To make the implementation less repetitive reimplemented `FlagsBase`
> with a variadic template; this allows to now use an arbitrary number of
> `FlagsBase` bases.
> 
> Since `Flags<>` does already inherit virtually from `FlagsBase` it
> should be a safer base class than `FlagsBase` for users.
> 
> Review: https://reviews.apache.org/r/41882/
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Till Toenshoff <to...@me.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review114186
-----------------------------------------------------------

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 242 - 257)
<https://reviews.apache.org/r/41882/#comment175010>

    Much slicker now, thanks for the suggestion Alexander!
    
    The comment helps understanding the ghist, gj!


- Till Toenshoff


On Jan. 13, 2016, 10:43 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2016, 10:43 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> To make the implementation less repetitive reimplemented `FlagsBase`
> with a variadic template; this allows to now use an arbitrary number of
> `FlagsBase` bases.
> 
> Since `Flags<>` does already inherit virtually from `FlagsBase` it
> should be a safer base class than `FlagsBase` for users.
> 
> Review: https://reviews.apache.org/r/41882/
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Benjamin Bannier <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/
-----------------------------------------------------------

(Updated Jan. 13, 2016, 11:43 a.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
-------

Used an improved impl (that e.g., is less repetitive).


Bugs: MESOS-4278
    https://issues.apache.org/jira/browse/MESOS-4278


Repository: mesos


Description (updated)
-------

To make the implementation less repetitive reimplemented `FlagsBase`
with a variadic template; this allows to now use an arbitrary number of
`FlagsBase` bases.

Since `Flags<>` does already inherit virtually from `FlagsBase` it
should be a safer base class than `FlagsBase` for users.

Review: https://reviews.apache.org/r/41882/


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 

Diff: https://reviews.apache.org/r/41882/diff/


Testing
-------


Thanks,

Benjamin Bannier

Re: Review Request 41882: Constrained types used in Flags instantiation.

Posted by Alexander Rojas <al...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41882/#review113717
-----------------------------------------------------------

Ship it!


Ship It!

- Alexander Rojas


On Jan. 11, 2016, 10:25 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41882/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 10:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Bugs: MESOS-4278
>     https://issues.apache.org/jira/browse/MESOS-4278
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Constrained types used in Flags instantiation.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp addef78ddeb0007cf1e1c79738381138a18a35b6 
> 
> Diff: https://reviews.apache.org/r/41882/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>