Re: Review Request 46821: Avoided slicing of flags in subprocess in libprocess and stout.

2016-08-25 Thread Michael Park 

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


Fix it, then Ship it!





3rdparty/libprocess/src/subprocess.cpp (line 149)


`if (flags != nullptr) {`


- Michael Park


On Aug. 25, 2016, 5:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46821/
> ---
> 
> (Updated Aug. 25, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While `FlagsBase` internally stores just maps of names and
> flags, the functions stored in a `Flag` rely on the original type of
> the `Flags` containing them (e.g., we perform dynamic casts to detect
> their types).
> 
> Since `Option` stores `T` as a value (i.e., it cannot contain
> reference types) any interface taking an `Option` cannot rely on
> polymorphic behavior of `T`. To make use of polymorphism we should
> instead store e.g., a pointer type to avoid slicing.
> 
> Here we change `Flags` arguments of `subprocess` to allow preserving
> the original type so `Flag` functions can work reliably; we do this by
> passing a non-owning pointer to a `Flags` so we do not restrict copying.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 83778bbfcc75f3f39b98d03a3580398e7d1062ea 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> e0c6b2d930fbd2cfe011e6faf44843b83ab1db27 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 3f3e21514bd5e2e388165eb64d540764097557ac 
> 
> Diff: https://reviews.apache.org/r/46821/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46821: Avoided slicing of flags in subprocess in libprocess and stout.

2016-07-04 Thread Benjamin Bannier 

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

(Updated July 5, 2016, 1:13 a.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Michael 
Park, and Vinod Kone.


Changes
---

Fixed ssl use case.


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


Repository: mesos


Description
---

While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).

Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.

Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` functions can work reliably; we do this by
passing a non-owning pointer to a `Flags` so we do not restrict copying.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
a929cc90fc9ee975f3635957518ced4eb70bdfeb 
  3rdparty/libprocess/include/process/subprocess_base.hpp 
e0c6b2d930fbd2cfe011e6faf44843b83ab1db27 
  3rdparty/libprocess/src/subprocess.cpp 
44073146118b6c6e9e730b8c901852594080a3eb 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
3f3e21514bd5e2e388165eb64d540764097557ac 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 46821: Avoided slicing of flags in subprocess in libprocess and stout.

2016-07-04 Thread Benjamin Bannier 

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

(Updated July 4, 2016, 2:41 p.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Michael 
Park, and Vinod Kone.


Changes
---

Pass `Flags` by raw ptr.


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


Repository: mesos


Description (updated)
---

While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).

Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.

Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` functions can work reliably; we do this by
passing a non-owning pointer to a `Flags` so we do not restrict copying.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
e0c6b2d930fbd2cfe011e6faf44843b83ab1db27 
  3rdparty/libprocess/src/subprocess.cpp 
44073146118b6c6e9e730b8c901852594080a3eb 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
3f3e21514bd5e2e388165eb64d540764097557ac 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 46821: Avoided slicing of flags in subprocess in libprocess and stout.

2016-07-03 Thread Benjamin Bannier 

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

(Updated July 3, 2016, 5:22 p.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Michael 
Park, and Vinod Kone.


Changes
---

Rebased.


Summary (updated)
-

Avoided slicing of flags in subprocess in libprocess and stout.


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


Repository: mesos


Description
---

While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).

Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.

Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` function can work reliably. We do this by
passing a `Shared` so we do not restrict copying; not that it
would also be possible to use an `Owned`, but this would
require an audit of all sites where the arguments are used as
`Owned` should not be copied, but do not prevent that on their own.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
e0c6b2d930fbd2cfe011e6faf44843b83ab1db27 
  3rdparty/libprocess/src/subprocess.cpp 
44073146118b6c6e9e730b8c901852594080a3eb 
  3rdparty/libprocess/src/subprocess_windows.cpp 
d4713ec9d6cdc3caca6e62d882c9efcb754c2017 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
3f3e21514bd5e2e388165eb64d540764097557ac 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 46821: Avoided slicing of flags in subprocess.

2016-05-11 Thread Benjamin Bannier 


> On May 11, 2016, 3:35 a.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 141
> > 
> >
> > Can you add a comment here on why we need to take an Option 
> > here? You can probably use the 2nd para in your description.

I updated the commit message.


- Benjamin


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


On May 11, 2016, 2:13 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46821/
> ---
> 
> (Updated May 11, 2016, 2:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Michael 
> Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While `FlagsBase` internally stores just maps of names and
> flags, the functions stored in a `Flag` rely on the original type of
> the `Flags` containing them (e.g., we perform dynamic casts to detect
> their types).
> 
> Since `Option` stores `T` as a value (i.e., it cannot contain
> reference types) any interface taking an `Option` cannot rely on
> polymorphic behavior of `T`. To make use of polymorphism we should
> instead store e.g., a pointer type to avoid slicing.
> 
> Here we change `Flags` arguments of `subprocess` to allow preserving
> the original type so `Flag` function can work reliably. We do this by
> passing a `Shared` so we do not restrict copying; not that it
> would also be possible to use an `Owned`, but this would
> require an audit of all sites where the arguments are used as
> `Owned` should not be copied, but do not prevent that on their own.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 6ce1a827e36e0c65985e3928c89f51561fdd70cd 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 727e940f12643974de4ff2734fba431b285b5de3 
> 
> Diff: https://reviews.apache.org/r/46821/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46821: Avoided slicing of flags in subprocess.

2016-05-11 Thread Benjamin Bannier 

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

(Updated May 11, 2016, 2:13 p.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Michael 
Park, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).

Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.

Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` function can work reliably. We do this by
passing a `Shared` so we do not restrict copying; not that it
would also be possible to use an `Owned`, but this would
require an audit of all sites where the arguments are used as
`Owned` should not be copied, but do not prevent that on their own.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess.hpp 
6ce1a827e36e0c65985e3928c89f51561fdd70cd 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
727e940f12643974de4ff2734fba431b285b5de3 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 46821: Avoided slicing of flags in subprocess.

2016-05-10 Thread Vinod Kone 

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



LGTM. Can you add @mpark or @joris to the reviews for a second look as well.


3rdparty/libprocess/include/process/subprocess.hpp (line 141)


Can you add a comment here on why we need to take an Option here? 
You can probably use the 2nd para in your description.


- Vinod Kone


On April 29, 2016, 9:37 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46821/
> ---
> 
> (Updated April 29, 2016, 9:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While `FlagsBase` internally stores just maps of names and
> flags, the functions stored in a `Flag` rely on the original type of
> the `Flags` containing them (e.g., we perform dynamic casts to detect
> their types).
> 
> Since `Option` stores `T` as a value (i.e., it cannot contain
> reference types) any interface taking an `Option` cannot rely on
> polymorphic behavior of `T`. To make use of polymorphism we should
> instead store e.g., a pointer type to avoid slicing.
> 
> Here we change `Flags` arguments of `subprocess` to allow preserving
> the original type so `Flag` function can work reliably.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 6ce1a827e36e0c65985e3928c89f51561fdd70cd 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 727e940f12643974de4ff2734fba431b285b5de3 
> 
> Diff: https://reviews.apache.org/r/46821/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>