This is an automatically generated e-mail. To reply, visit:


    Let's just change this everywhere to EXIT_SUCCESS but without the comment.




    I'm not convinced the 'errorMessage' is more help here. You're basically 
saving doing a 'return EXIT_FAILURE;' in each of these if's, which is not a big 
deal and arguably more explicit and clear. On the other hand, this code was 
originally meant to be structured so the line that did a 'flags.master.get()' 
was as close to the line that checked 'flags.master.isSome()' and by moving 
that code farther away it makes it so a reader has to look harder to confirm 
that when someone is "dereferencing" 'flags.master' it's indeed safe.


    This pattern is why we use Option!




    I liked your pattern of doing 'setUsageMessage' immediately after 
instantiating the class. Any reason not to do that here?


    Let's also use this review as an opportunity to do 'EXIT_FAILURE' anyplace 
we 'return -1' and 'EXIT_SUCCESS' anyplace we 'return 0'. Please do a sweep in 
every file you've touched and make it so, thanks Marco!


    Another request: let's replace 'exit(1)' with 'return EXIT_FAILURE' here 
and everywhere else. Likewise 'exit(0)' with 'return EXIT_SUCCESS'.


    I see that you changed some of these 'EXIT()' calls below to be:
    cerr << flags.usage(message) << endl;
    return EXIT_FAILURE;
    But not all of them have been changed, like these ones. I'd like to make 
things consistent now even if we weren't consistent in the past.


    How come this 'usage' isn't getting killed?


    Here's another example of an EXIT() that we can change. Note that I don't 
so much mind:
    EXIT(EXIT_FAILURE) << flags.usage(load.error());
    But let's just be consistent please.




    To capture what we were printing before, this should be:
    "Usage: " + os::basename(argv[0]).get() << " [...]\n\n" +
    "Launches an in-memory cluster within a single process.";


    Why are we not killing this 'usage'?


    Why are we not killing this 'usage'?


    Please end sentence with period (here and everywhere please).


    s/ERROR: //

- Benjamin Hindman

On May 28, 2015, 10:06 a.m., Marco Massenzio wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
> (Updated May 28, 2015, 10:06 a.m.)
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> Bugs: MESOS-2711
>     https://issues.apache.org/jira/browse/MESOS-2711
> Repository: mesos
> Description
> -------
> Jira: MESOS-2711
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
> This touches a lot of files, but keep scrolling, and you will see a pattern 
> emerge.
> Diffs
> -----
>   src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17 
>   src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b 
>   src/examples/load_generator_framework.cpp 
> be1a3bf5f16bd811cb4039c8f15478183712a426 
>   src/examples/persistent_volume_framework.cpp 
> 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65 
>   src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3 
>   src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c 
>   src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589 
>   src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb 
>   src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442 
>   src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f 
>   src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125 
>   src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> c72fb47f60f40cda8d84a10497b9133f83cf018e 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 49e983edab598e2ac487bb488fdd12840a9e7dfc 
>   src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4 
>   src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0 
>   src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902 
> Diff: https://reviews.apache.org/r/34195/diff/
> Testing
> -------
> make check
> **NOTE** this fixes completely the chained changes from 34193 and makes all 
> the tests pass.
> Thanks,
> Marco Massenzio

Reply via email to