Re: Review Request 46621: Add alias support for flags.

2016-04-25 Thread Vinod Kone 


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > Did a quick review since I spent some time on working on a fix for 
> > MESOS-3335. It would be great if we wouldn't aggreviate that problem 
> > further.
> > 
> > What seems unclear to me ATM is how alias'ed flags can override each other, 
> > or what the preferred order would be.

There is no override or preference order. Only one of name/alias is allowed. 
Otherwise we throw an error.


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp, line 64
> > 
> >
> > Does it makes sense to support multiple aliases?

that was how the original patch was in my branch. but reverted it because i 
didn't have any use cases to use multiple aliases. we can add it later if we 
need it.


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp, line 81
> > 
> >
> > We could return a const ref here.

just as an optmization? don't think we do that in our code base.


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 152-171
> > 
> >
> > These suffer from the problem described in MESOS-3335.

yes. but IIUC this issue is not introduced here>? i would rather we fix it in 
one swoop when we fix MESOS-3335.


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 192-200
> > 
> >
> > This also suffers from the problem described in MESOS-3335.

see above.


> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 212-216
> > 
> >
> > This also suffers from the problem described in MESOS-3335.

see above.


- Vinod


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


On April 25, 2016, 5:44 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> ---
> 
> (Updated April 25, 2016, 5:44 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add alias support for flags.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> c3cbcdb781e1c282d381de1ad2bf4f386ee1db21 
> 
> Diff: https://reviews.apache.org/r/46621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 46621: Add alias support for flags.

2016-04-25 Thread Benjamin Bannier 

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



Did a quick review since I spent some time on working on a fix for MESOS-3335. 
It would be great if we wouldn't aggreviate that problem further.

What seems unclear to me ATM is how alias'ed flags can override each other, or 
what the preferred order would be.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 64)


Does it makes sense to support multiple aliases?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 81)


We could return a const ref here.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 152 - 
171)


These suffer from the problem described in MESOS-3335.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 192 - 
200)


This also suffers from the problem described in MESOS-3335.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 212 - 
216)


This also suffers from the problem described in MESOS-3335.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 232 - 
250)


These signatures do not in general suffer from the problem of MESOS-3335 
since they use a member pointer instead of a pointer to (at some point) random 
memory.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 272 - 
273)


Signature also safe re:MESOS-3335.


- Benjamin Bannier


On April 25, 2016, 7:44 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> ---
> 
> (Updated April 25, 2016, 7:44 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add alias support for flags.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> c3cbcdb781e1c282d381de1ad2bf4f386ee1db21 
> 
> Diff: https://reviews.apache.org/r/46621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>