> On May 18, 2015, 10:04 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 633 > > <https://reviews.apache.org/r/34193/diff/2/?file=963013#file963013line633> > > > > 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 > > <https://reviews.apache.org/r/34193/diff/2/?file=963013#file963013line637> > > > > 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 > > <https://reviews.apache.org/r/34193/diff/2/?file=963014#file963014line511> > > > > 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: <prog name> > [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 > >
