Re: Review Request 34193: Refactored common functionality into BaseFlags

[Benjamin Hindman]

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


One thing I missed in my last review, let's make sure we s/BaseFlags/FlagsBase/ 
everywhere in this review (and Summary, etc). ;-)

- Benjamin Hindman


On May 20, 2015, 11:22 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 20, 2015, 11:22 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 151
> > 
> >
> > I think it would be great if we could consistently use the trailing 
> > underscore it for any single class, rather than some fields using it and 
> > some not as this could confuse people when to use it. In this case, any 
> > reason not to change all the fields or not use the trailing underscore for 
> > now?

Funny you mention that, as this pained me too.
In our current practice, I discovered, we seem to discourage people from fixing 
this kind of "localized, hence easy while I'm changing this code" issues - and 
rather punt to an indeterminate future time by "creating a Jira ticket" (in 
this case the time of doing so, coupled with the cost of the context switch is 
ridiculously higher than the two keystrokes required in any modern IDE to fix 
all the private variables' names).

I found myself caught between two bad options:
1. risk everyone's ire by executing a fix that was not sanctioned; or
2. perpetuating a violation of the code style.

Just say the word, and I'll happily refactor all private variable names (or 
just remove the trailing underscore from `programName_` - but this will require 
special dispensation, as it violates the current style guide).

Needless to say, I'd be happier with the former.


> On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 92-103
> > 
> >
> > Adhering to the Google Style Guide, we do not use non-const reference 
> > arguments to functions: 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments
> > 
> > As the style guide points out, there are exceptions here, most notably 
> > 'operator <<' which, by no coincidence, is relevant here and perhaps why 
> > this slipped through? ;-)
> > 
> > Here's my suggestion to resolve this issue: let's add the 'std::string 
> > message' to 'usage()'. There wasn't anything special about that function 
> > that didn't include it originally, so I don't see any reason to not add it 
> > now. Here's how this would look:
> > 
> > std::string usage(const Option& message = None()) const;
> > 
> > Now in the body of 'usage' we can add our default message or replace 
> > with whatever someone else passed in. And then here is how it would look 
> > like when used:
> > 
> > if (flags.help) {
> >   std::cerr << flags.usage("This is: TestFlags [options]") << std::endl;
> > }
> > 
> > The alternative here would be to take std::ostream as a pointer, but 
> > where ever we can cleanly prefer the functional style we should as it leads 
> > to more composable and easier to reason about code (i.e., avoiding 
> > non-local manipulation of variables, etc).
> > 
> > Moreover, I actually think it yields cleaner code at the call sites 
> > where we are already using std::cerr directly, for example:
> > 
> > if (flags.master.isNone()) {
> >   std::cerr << "Missing required option --master" << std::endl;
> >   flags.printUsage(std::cerr);
> >   return EXIT_FAILURE;
> > }
> > 
> > Would be cleaner to me as:
> > 
> > if (flags.master.isNone()) {
> >   std::cerr << "Missing required option --master" << std::endl
> > << flags.usage() << std::endl;
> >   return EXIT_FAILURE;
> > }
> > 
> > Finally, to cover all the cases here, I could imagine killing 
> > 'setUsageMsg' and letting folks define their own constants that they always 
> > pass in, for example:
> > 
> > const string USAGE_MESSAGE = "This is: TestFlags [Options]";
> > 
> > ...
> > 
> > std::cerr << flags.usage(USAGE_MESSAGE) << std::endl;
> > 
> > 
> > To be clear I'm not opposed to having a usage message setter, but I'm 
> > always in favor of having less ways, or ideally one way, of doing the same 
> > thing. And regardless, s/setUsageMsg/setUsageMessage/ (it looks like Joris 
> > mentioned this in a previous review, so just following up here, thanks 
> > guys!).

Just to be clear, it didn't "slip through" :) this was by design (`operator <<` 
is non-const and also I need to return the ostream as a non-const ref).

I quite like your suggestion, though - I can easily implement it.
Just to be clear, your last example could have actually also been:
```
if (flags.master.isNone()) {
  std::cerr <<  flags.usage("Missing required option --master") << std::endl;
  return EXIT_FAILURE;
}

```
am I getting this right?

By also adding a `protected: string defaultUsageMessage_` (which would be 
initialized in the constructor to be equal to `const string 
FlagsBase::DEFAULT_USAGE_MESSAGE`) derived classes could change just that

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Benjamin Hindman]

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

Ship it!


Everything looks good but let's please resolve the issue with 'printUsage' and 
then I'll commit.


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


Adhering to the Google Style Guide, we do not use non-const reference 
arguments to functions: 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments

As the style guide points out, there are exceptions here, most notably 
'operator <<' which, by no coincidence, is relevant here and perhaps why this 
slipped through? ;-)

Here's my suggestion to resolve this issue: let's add the 'std::string 
message' to 'usage()'. There wasn't anything special about that function that 
didn't include it originally, so I don't see any reason to not add it now. 
Here's how this would look:

std::string usage(const Option& message = None()) const;

Now in the body of 'usage' we can add our default message or replace with 
whatever someone else passed in. And then here is how it would look like when 
used:

if (flags.help) {
  std::cerr << flags.usage("This is: TestFlags [options]") << std::endl;
}

The alternative here would be to take std::ostream as a pointer, but where 
ever we can cleanly prefer the functional style we should as it leads to more 
composable and easier to reason about code (i.e., avoiding non-local 
manipulation of variables, etc).

Moreover, I actually think it yields cleaner code at the call sites where 
we are already using std::cerr directly, for example:

if (flags.master.isNone()) {
  std::cerr << "Missing required option --master" << std::endl;
  flags.printUsage(std::cerr);
  return EXIT_FAILURE;
}

Would be cleaner to me as:

if (flags.master.isNone()) {
  std::cerr << "Missing required option --master" << std::endl
<< flags.usage() << std::endl;
  return EXIT_FAILURE;
}

Finally, to cover all the cases here, I could imagine killing 'setUsageMsg' 
and letting folks define their own constants that they always pass in, for 
example:

const string USAGE_MESSAGE = "This is: TestFlags [Options]";

...

std::cerr << flags.usage(USAGE_MESSAGE) << std::endl;

To be clear I'm not opposed to having a usage message setter, but I'm 
always in favor of having less ways, or ideally one way, of doing the same 
thing. And regardless, s/setUsageMsg/setUsageMessage/ (it looks like Joris 
mentioned this in a previous review, so just following up here, thanks guys!).



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


I think it would be great if we could consistently use the trailing 
underscore it for any single class, rather than some fields using it and some 
not as this could confuse people when to use it. In this case, any reason not 
to change all the fields or not use the trailing underscore for now?



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


Any reason not to make 'programName_' be an Option rather than setting it 
to the empty string if argv[0] doesn't exist? While I understand in this case 
we'd probably print the same thing, we definitely have more information with an 
Option versus someone passing us an empty string for argv[0].



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp


Two newlines between all top-level definitions/declarations please! I see 
Joris called this out in a previous review but he just addressed a single place 
instead of all the places. ;-) Thanks guys!



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp


With the revised 'usage()' this will just be:

out << flags.usage("This is: TestFlags [options]");


- Benjamin Hindman


On May 20, 2015, 11:22 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 20, 2015, 11:22 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functional

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.
> 
> Marco Massenzio wrote:
> I'm not sure I follow the logic here, Joris - I'll go along with it, as 
> an extra 20 keystrokes or so are not something I want to make a big issue 
> about, but it seems warped; I also note here, that I'm adding the `using` 
> clause some several 100's lines above where it's used, and that goes against 
> the whole concept of readability.
> 
> Marco Massenzio wrote:
> BTW - just came across this:
> ```
> std::string pad(PAD + width - line.size(), ' ');
> line += pad;
> 
> size_t pos1 = 0, pos2 = 0;
> // ...
> while (pos2 != std::string::npos) {  
> ```
> in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> IMO this only makes sense if you use the same type/variable several times 
> and/or typing up the full namespace is tedious: `std::` is actually pretty 
> low overhead.
> 
> Michael Park wrote:
> Our style guide isn't unclear on the use of `using`. But regarding the 
> following statement:
> > in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> 
> This is because `flags.hpp` is a header file. It's very bad practice to 
> use `using` declarations in a header file, since anything that includes it 
> will implicitly inherit the declarations. Esepcially in a library header like 
> `stout` which is included pretty much everywhere. Note that Joris qualified 
> his statement with: "Since we're in an implementation file"
> 
> I personally don't have strong opinions about this. However, looking 
> through the rest of the codebase, at least for `std::ostringstream` in 
> particular, there are 9 cases where we do `using std::ostringstream;` and 
> refer to `ostringstream` only once.
> 
> ```
> src/exmaples/persistent_volume_framework.cpp
> src/linux/perf.cpp
> src/log/tool/benchmark.cpp
> src/log/tool/initialize.cpp
> src/log/tool/read.cpp
> src/log/tool/replica.cpp
> src/slave/containerizer/isolators/cgroups/mem.cpp
> src/slave/containerizer/isolators/network/port_mapping.cpp
> src/tests/isolator_tests.cpp
> ```
> 
> As opposed to 3 instances of `std::ostringstream` being referenced once 
> in the file.
> 
> ```
> 3rdparty/libprocess/src/pid.cpp
> 3rdparty/libprocess/src/process.cpp
> 3rdparty/libprocess/src/tests/http_tests.cpp
> ```
> 
> The review comments in the past from folks have been to declare the 
> `using` at the top (albeit not formalized), so I would err on that side in 
> this case. Our standard approach is to: follow what at least seems to be 
> majority in the codebase, come up with a concrete style guide (e.g. "used in 
> several places" -- how many is several?) and create a JIRA ticket for the 
> consistent update around the codebase.
> 
> Marco Massenzio wrote:
> So, you went through all that trouble, how about just looking a few lines 
> down 
> [L597](https://git-wip-us.apache.org/repos/asf?p=mesos.git;a=blob;f=3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp;h=00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1;hb=f1b3c095ef96bc2f1e2f3698ab4eb0413acef4fa#l597):
> ```
> Option something;
> ```
> or L574:
> ```
> JSON::Object nested;
> ```
> How are those examples less relevant than what you found elsewhere?
> Or should we agree that both approaches are valid; neither is preferable; 
> and trying to be "consistent" *in this particular instance* is immaterial?
> 
> Joris Van Remoortere wrote:
> L597 is a great example of something we need to clean up that snuck 
> through. Let's fix it!
> In this file there is already a `using std::string` declaration, so we 
> should cut this short to `string`.
> 
> The reason we are trying to lean in a particular direction is the "Broken 
> Window Principle". Due to the complexity of parsing c++, our tools do not yet 
> cover all the cases we want. This inevitably leads to inconsistent code 
> getting through. That is not a reason to let it slide when we do catch it ;-)

Well - I

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Joris Van Remoortere]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 45
> > 
> >
> > It is best practice to make base class constructors protected to 
> > prevent accidental instantiation of a non-final class.
> 
> Marco Massenzio wrote:
> BTW - this causes a compile error:
> ```
> In file included from 
> ../../../../3rdparty/libprocess/3rdparty/stout/tests/subcommand_tests.cpp:25:
> 
> ../../../../3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp:78:12:
>  error: field of type 'flags::FlagsBase' has
>   protected default constructor
>   explicit Subcommand(const std::string& _name) : name_(_name) {}
> ```
> also, at least in theory, there's nothing stopping someone to use 
> directly `FlagsBase` (and call `add()` on it): it can be used by itself.

Indeed, it causes a compilation error. This is because subcommand is 
instantiating the base class!
Let's make a leaf class to fix this.
I've added JIRA:MESOS-2756 to address the issue of object slicing and public 
construction of base classes (the anti-pattern mentioned above).


> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.
> 
> Marco Massenzio wrote:
> I'm not sure I follow the logic here, Joris - I'll go along with it, as 
> an extra 20 keystrokes or so are not something I want to make a big issue 
> about, but it seems warped; I also note here, that I'm adding the `using` 
> clause some several 100's lines above where it's used, and that goes against 
> the whole concept of readability.
> 
> Marco Massenzio wrote:
> BTW - just came across this:
> ```
> std::string pad(PAD + width - line.size(), ' ');
> line += pad;
> 
> size_t pos1 = 0, pos2 = 0;
> // ...
> while (pos2 != std::string::npos) {  
> ```
> in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> IMO this only makes sense if you use the same type/variable several times 
> and/or typing up the full namespace is tedious: `std::` is actually pretty 
> low overhead.
> 
> Michael Park wrote:
> Our style guide isn't unclear on the use of `using`. But regarding the 
> following statement:
> > in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> 
> This is because `flags.hpp` is a header file. It's very bad practice to 
> use `using` declarations in a header file, since anything that includes it 
> will implicitly inherit the declarations. Esepcially in a library header like 
> `stout` which is included pretty much everywhere. Note that Joris qualified 
> his statement with: "Since we're in an implementation file"
> 
> I personally don't have strong opinions about this. However, looking 
> through the rest of the codebase, at least for `std::ostringstream` in 
> particular, there are 9 cases where we do `using std::ostringstream;` and 
> refer to `ostringstream` only once.
> 
> ```
> src/exmaples/persistent_volume_framework.cpp
> src/linux/perf.cpp
> src/log/tool/benchmark.cpp
> src/log/tool/initialize.cpp
> src/log/tool/read.cpp
> src/log/tool/replica.cpp
> src/slave/containerizer/isolators/cgroups/mem.cpp
> src/slave/containerizer/isolators/network/port_mapping.cpp
> src/tests/isolator_tests.cpp
> ```
> 
> As opposed to 3 instances of `std::ostringstream` being referenced once 
> in the file.
> 
> ```
> 3rdparty/libprocess/src/pid.cpp
> 3rdparty/libprocess/src/process.cpp
> 3rdparty/libprocess/src/tests/http_tests.cpp
> ```
> 
> The review comments in the past from folks have been to declare the 
> `using` at the top (albeit not formalized), so I would err on that side in 
> this case. Our standard approach is to: follow what at least seems to be 
> majority in the codebase, come up with a concrete style guide (e.g. "used in 
> several places" -- how many is several?) and create a JIRA ticket for the 
> consistent update around the codebase.
> 
> Marco Massenzio wr

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

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

(Updated May 20, 2015, 11:22 p.m.)


Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Changes
---

More comments changes


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


Repository: mesos


Description
---

Jira: MESOS-2711

Every program that uses stout's `BaseFlags` ends up
re-implementing the `printUsage()` function, and adding
a `bool help` (and associated --help flag); this functionality
has now been refactored in the base class and is available everywhere.

This change attempts to be backward-compatible, so it
does not alter the behavior of the program when --help is
invoked (by, eg, automatically printing usage and exiting)
but leaves up to the caller to check for `flags.help` and then
decide what action to take.

There is now a default behavior for the "leader" ("Usage:  
[options]")
but the client API allows to modify that too.

Note - anywhere I found the use of the `--help` flag the behavior was the same: 
print usage and exit (see also https://reviews.apache.org/r/34195).


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
fb383b463a99924483634eebf22bf34de318f920 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 

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


Testing
---

make check

**NOTE** this change by itself breaks the build, because the --help is 
redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
makes all build/tests pass.


Thanks,

Marco Massenzio

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.
> 
> Marco Massenzio wrote:
> I'm not sure I follow the logic here, Joris - I'll go along with it, as 
> an extra 20 keystrokes or so are not something I want to make a big issue 
> about, but it seems warped; I also note here, that I'm adding the `using` 
> clause some several 100's lines above where it's used, and that goes against 
> the whole concept of readability.
> 
> Marco Massenzio wrote:
> BTW - just came across this:
> ```
> std::string pad(PAD + width - line.size(), ' ');
> line += pad;
> 
> size_t pos1 = 0, pos2 = 0;
> // ...
> while (pos2 != std::string::npos) {  
> ```
> in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> IMO this only makes sense if you use the same type/variable several times 
> and/or typing up the full namespace is tedious: `std::` is actually pretty 
> low overhead.
> 
> Michael Park wrote:
> Our style guide isn't unclear on the use of `using`. But regarding the 
> following statement:
> > in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> 
> This is because `flags.hpp` is a header file. It's very bad practice to 
> use `using` declarations in a header file, since anything that includes it 
> will implicitly inherit the declarations. Esepcially in a library header like 
> `stout` which is included pretty much everywhere. Note that Joris qualified 
> his statement with: "Since we're in an implementation file"
> 
> I personally don't have strong opinions about this. However, looking 
> through the rest of the codebase, at least for `std::ostringstream` in 
> particular, there are 9 cases where we do `using std::ostringstream;` and 
> refer to `ostringstream` only once.
> 
> ```
> src/exmaples/persistent_volume_framework.cpp
> src/linux/perf.cpp
> src/log/tool/benchmark.cpp
> src/log/tool/initialize.cpp
> src/log/tool/read.cpp
> src/log/tool/replica.cpp
> src/slave/containerizer/isolators/cgroups/mem.cpp
> src/slave/containerizer/isolators/network/port_mapping.cpp
> src/tests/isolator_tests.cpp
> ```
> 
> As opposed to 3 instances of `std::ostringstream` being referenced once 
> in the file.
> 
> ```
> 3rdparty/libprocess/src/pid.cpp
> 3rdparty/libprocess/src/process.cpp
> 3rdparty/libprocess/src/tests/http_tests.cpp
> ```
> 
> The review comments in the past from folks have been to declare the 
> `using` at the top (albeit not formalized), so I would err on that side in 
> this case. Our standard approach is to: follow what at least seems to be 
> majority in the codebase, come up with a concrete style guide (e.g. "used in 
> several places" -- how many is several?) and create a JIRA ticket for the 
> consistent update around the codebase.

So, you went through all that trouble, how about just looking a few lines down 
[L597](https://git-wip-us.apache.org/repos/asf?p=mesos.git;a=blob;f=3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp;h=00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1;hb=f1b3c095ef96bc2f1e2f3698ab4eb0413acef4fa#l597):
```
Option something;
```
or L574:
```
JSON::Object nested;
```
How are those examples less relevant than what you found elsewhere?
Or should we agree that both approaches are valid; neither is preferable; and 
trying to be "consistent" *in this particular instance* is immaterial?


- Marco


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2711
> https:

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Michael Park]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.
> 
> Marco Massenzio wrote:
> I'm not sure I follow the logic here, Joris - I'll go along with it, as 
> an extra 20 keystrokes or so are not something I want to make a big issue 
> about, but it seems warped; I also note here, that I'm adding the `using` 
> clause some several 100's lines above where it's used, and that goes against 
> the whole concept of readability.
> 
> Marco Massenzio wrote:
> BTW - just came across this:
> ```
> std::string pad(PAD + width - line.size(), ' ');
> line += pad;
> 
> size_t pos1 = 0, pos2 = 0;
> // ...
> while (pos2 != std::string::npos) {  
> ```
> in flags.hpp - so it would appear that using namespace-scoped variables 
> if just fine (at least in Stout)?
> IMO this only makes sense if you use the same type/variable several times 
> and/or typing up the full namespace is tedious: `std::` is actually pretty 
> low overhead.

Our style guide isn't unclear on the use of `using`. But regarding the 
following statement:
> in flags.hpp - so it would appear that using namespace-scoped variables if 
> just fine (at least in Stout)?

This is because `flags.hpp` is a header file. It's very bad practice to use 
`using` declarations in a header file, since anything that includes it will 
implicitly inherit the declarations. Esepcially in a library header like 
`stout` which is included pretty much everywhere. Note that Joris qualified his 
statement with: "Since we're in an implementation file"

I personally don't have strong opinions about this. However, looking through 
the rest of the codebase, at least for `std::ostringstream` in particular, 
there are 9 cases where we do `using std::ostringstream;` and refer to 
`ostringstream` only once.

```
src/exmaples/persistent_volume_framework.cpp
src/linux/perf.cpp
src/log/tool/benchmark.cpp
src/log/tool/initialize.cpp
src/log/tool/read.cpp
src/log/tool/replica.cpp
src/slave/containerizer/isolators/cgroups/mem.cpp
src/slave/containerizer/isolators/network/port_mapping.cpp
src/tests/isolator_tests.cpp
```

As opposed to 3 instances of `std::ostringstream` being referenced once in the 
file.

```
3rdparty/libprocess/src/pid.cpp
3rdparty/libprocess/src/process.cpp
3rdparty/libprocess/src/tests/http_tests.cpp
```

The review comments in the past from folks have been to declare the `using` at 
the top (albeit not formalized), so I would err on that side in this case. Our 
standard approach is to: follow what at least seems to be majority in the 
codebase, come up with a concrete style guide (e.g. "used in several places" -- 
how many is several?) and create a JIRA ticket for the consistent update around 
the codebase.


- Michael


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I f

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 45
> > 
> >
> > It is best practice to make base class constructors protected to 
> > prevent accidental instantiation of a non-final class.

BTW - this causes a compile error:
```
In file included from 
../../../../3rdparty/libprocess/3rdparty/stout/tests/subcommand_tests.cpp:25:
../../../../3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp:78:12:
 error: field of type 'flags::FlagsBase' has
  protected default constructor
  explicit Subcommand(const std::string& _name) : name_(_name) {}
```
also, at least in theory, there's nothing stopping someone to use directly 
`FlagsBase` (and call `add()` on it): it can be used by itself.


- Marco


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.
> 
> Marco Massenzio wrote:
> I'm not sure I follow the logic here, Joris - I'll go along with it, as 
> an extra 20 keystrokes or so are not something I want to make a big issue 
> about, but it seems warped; I also note here, that I'm adding the `using` 
> clause some several 100's lines above where it's used, and that goes against 
> the whole concept of readability.

BTW - just came across this:
```
std::string pad(PAD + width - line.size(), ' ');
line += pad;

size_t pos1 = 0, pos2 = 0;
// ...
while (pos2 != std::string::npos) {  
```
in flags.hpp - so it would appear that using namespace-scoped variables if just 
fine (at least in Stout)?
IMO this only makes sense if you use the same type/variable several times 
and/or typing up the full namespace is tedious: `std::` is actually pretty low 
overhead.


- Marco


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 633
> > 
> >
> > Did you substitute the `std::endl` with `\n\n` on purpose? Why not stay 
> > consistent and use `std::endl` for all cases?
> 
> Marco Massenzio wrote:
> actually, `std::endl` does more than just add a `\n` - it also flushes 
> the buffer (which is unnecessary here).
> I think I've read something to the effect of avoiding `endl` as the 
> default newline, unless specifically wanted - but it could well be bogus.
> 
> There is this: http://stackoverflow.com/questions/213907/c-stdendl-vs-n 
> or this (more vocal :): 
> http://kuhllib.com/2012/01/14/stop-excessive-use-of-stdendl/
> 
> One of those habits I've picked over the years (preferring `\n` to 
> `std::endl`) but I'm happy to change it, if there's a good reason for it.
> 
> Joris Van Remoortere wrote:
> IIUC there is a subtle platform dependence difference in what endl maps 
> to, depending on what the underlying stream maps to. Since we don't know what 
> the stream maps to by the time we are appending to it, the conversion could 
> be inconsistent.
> I don't think this function is particularly a bottle neck in Mesos, so I 
> would lean towards consistency over premature optimization here.

sure, no problem
however, if we are "banning" `\n` we should codify in the style guide, IMO


> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.
> 
> Joris Van Remoortere wrote:
> I'm not sure if we have an explicit rule here for this. I just know if 
> someone else comes to review this code, they will point out the same thing. 
> Let's just go with consistency.

I'm not sure I follow the logic here, Joris - I'll go along with it, as an 
extra 20 keystrokes or so are not something I want to make a big issue about, 
but it seems warped; I also note here, that I'm adding the `using` clause some 
several 100's lines above where it's used, and that goes against the whole 
concept of readability.


- Marco


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

> On May 20, 2015, 8:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 383-387
> > 
> >
> > What about:
> > ```
> > programName_ = argc > 0 ? os::basename(argv[0]) : "";
> > ```
> > 
> > I assume you want to explicitly set the empty string in case something 
> > else has touched `programName_` before we call this function. I think this 
> > is great safe programming :-)

ok - thanks (for this and catching the above)
I was actually "copying" a pattern I'd seen elsewhere in this file (I think) 
but maybe I got it wrong.
This looks cleaner.


> On May 20, 2015, 8:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 95-99
> > 
> >
> > I can't find other places in the code where we use a backtick rather 
> > than a single quote. Would you mind staying consistent? This makes it 
> > easier to refactor / search for code later.
> > 
> > Here and elsehwere in the review.

not sure I agree with the logic, but, sure, why not.


- Marco


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Joris Van Remoortere]

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



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


I can't find other places in the code where we use a backtick rather than a 
single quote. Would you mind staying consistent? This makes it easier to 
refactor / search for code later.

Here and elsehwere in the review.



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


This won't compile for 2 reasons:
1) use `std::string` in the header
2) use `None()` instead of `Nothing()`

Same below.



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


What about:
```
programName_ = argc > 0 ? os::basename(argv[0]) : "";
```

I assume you want to explicitly set the empty string in case something else 
has touched `programName_` before we call this function. I think this is great 
safe programming :-)


- Joris Van Remoortere


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 
> 
> Diff: https://reviews.apache.org/r/34193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> **NOTE** this change by itself breaks the build, because the --help is 
> redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
> makes all build/tests pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Joris Van Remoortere]

> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 633
> > 
> >
> > Did you substitute the `std::endl` with `\n\n` on purpose? Why not stay 
> > consistent and use `std::endl` for all cases?
> 
> Marco Massenzio wrote:
> actually, `std::endl` does more than just add a `\n` - it also flushes 
> the buffer (which is unnecessary here).
> I think I've read something to the effect of avoiding `endl` as the 
> default newline, unless specifically wanted - but it could well be bogus.
> 
> There is this: http://stackoverflow.com/questions/213907/c-stdendl-vs-n 
> or this (more vocal :): 
> http://kuhllib.com/2012/01/14/stop-excessive-use-of-stdendl/
> 
> One of those habits I've picked over the years (preferring `\n` to 
> `std::endl`) but I'm happy to change it, if there's a good reason for it.

IIUC there is a subtle platform dependence difference in what endl maps to, 
depending on what the underlying stream maps to. Since we don't know what the 
stream maps to by the time we are appending to it, the conversion could be 
inconsistent.
I don't think this function is particularly a bottle neck in Mesos, so I would 
lean towards consistency over premature optimization here.


> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 637
> > 
> >
> > let's add a blank line between the closing of the branch and the return 
> > statement.
> 
> Marco Massenzio wrote:
> I do have a problem with that - and so does the [Google Style 
> Guide](https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Vertical_Whitespace)
> ```
> Minimize use of vertical whitespace.
> ```
> Especially in files hundreds of lines long, adding empty lines makes them 
> even less readable (as one has to scroll for longer)
> I actually think that adding an empty line does not make this method more 
> readable (or prettier to look at).

This is a pattern used in the Mesos code base. Let's be consistent with it for 
now, and open a separate discussion / JIRA for changing this pattern to follow 
the google style guide more clearly if you like.


> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 511
> > 
> >
> > Since we're in an implementation file, we can `using 
> > std::ostringstream;` and then just use `ostringstream` here.
> 
> Marco Massenzio wrote:
> do I have to?
> I don't particularly like the `using` thing (although appreciate its 
> usefulness) and avoid using (ha, a pun!) it unless the type is used in 
> several places, in which case, the gain is significant.

I'm not sure if we have an explicit rule here for this. I just know if someone 
else comes to review this code, they will point out the same thing. Let's just 
go with consistency.


- Joris


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


On May 19, 2015, 10:31 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 19, 2015, 10:31 p.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
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functionality
> has now been refactored in the base class and is available everywhere.
> 
> This change attempts to be backward-compatible, so it
> does not alter the behavior of the program when --help is
> invoked (by, eg, automatically printing usage and exiting)
> but leaves up to the caller to check for `flags.help` and then
> decide what action to take.
> 
> There is now a default behavior for the "leader" ("Usage:  
> [options]")
> but the client API allows to modify that too.
> 
> Note - anywhere I found the use of the `--help` flag the behavior was the 
> same: 
> print usage and exit (see also https://reviews.apache.org/r/34195).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> fb383b463a99924483634eebf22bf34de318f920 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

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

(Updated May 19, 2015, 10:31 p.m.)


Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Changes
---

Addressed Joris comments


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


Repository: mesos


Description
---

Jira: MESOS-2711

Every program that uses stout's `BaseFlags` ends up
re-implementing the `printUsage()` function, and adding
a `bool help` (and associated --help flag); this functionality
has now been refactored in the base class and is available everywhere.

This change attempts to be backward-compatible, so it
does not alter the behavior of the program when --help is
invoked (by, eg, automatically printing usage and exiting)
but leaves up to the caller to check for `flags.help` and then
decide what action to take.

There is now a default behavior for the "leader" ("Usage:  
[options]")
but the client API allows to modify that too.

Note - anywhere I found the use of the `--help` flag the behavior was the same: 
print usage and exit (see also https://reviews.apache.org/r/34195).


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
fb383b463a99924483634eebf22bf34de318f920 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 

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


Testing
---

make check

**NOTE** this change by itself breaks the build, because the --help is 
redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
makes all build/tests pass.


Thanks,

Marco Massenzio

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Marco Massenzio]

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

(Updated May 18, 2015, 5:26 p.m.)


Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Repository: mesos


Description
---

Jira: MESOS-2711

Every program that uses stout's `BaseFlags` ends up
re-implementing the `printUsage()` function, and adding
a `bool help` (and associated --help flag); this functionality
has now been refactored in the base class and is available everywhere.

This change attempts to be backward-compatible, so it
does not alter the behavior of the program when --help is
invoked (by, eg, automatically printing usage and exiting)
but leaves up to the caller to check for `flags.help` and then
decide what action to take.

There is now a default behavior for the "leader" ("Usage:  
[options]")
but the client API allows to modify that too.

Note - anywhere I found the use of the `--help` flag the behavior was the same: 
print usage and exit (see also https://reviews.apache.org/r/34195).


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
fb383b463a99924483634eebf22bf34de318f920 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 

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


Testing
---

make check

**NOTE** this change by itself breaks the build, because the --help is 
redefined everywhere (16 places, as of last count): CL 34195 fixes that and 
makes all build/tests pass.


Thanks,

Marco Massenzio