> 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. > > 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 > > <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. > > 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: <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 > >
