mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 34193: Refactored common functionality into BaseFlags
Date Wed, 20 May 2015 19:45:58 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.

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


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