mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34193: Refactored common functionality into BaseFlags
Date Wed, 20 May 2015 20:15:00 GMT


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


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