mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 46621: Add alias support for flags.
Date Tue, 26 Apr 2016 01:03:10 GMT


> 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
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358750#file1358750line64>
> >
> >     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
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358750#file1358750line81>
> >
> >     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
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line152>
> >
> >     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
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line192>
> >
> >     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
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line212>
> >
> >     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
> 
>


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