> 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. > > 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<std::string> 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 think you can hardly call `std::ostringstream buf;` a "broken window" - it does not violate the code style; it does not detract from readability; it does not impact your ability to RegEx your way through the code (as proved valiantly by mpark ;-); and it does not add any maintenance burden. I am also not entirely sure that cleaning the `std::string` would either, but I'm happy to give it the benefit of the doubt. I think you are confusing your style preferences with some absolutes. (BTW - I was happy to give in on this point, but your "Broken Windows" reference made me realize that maybe Ben and I should elaborate on the point) An example of a "Broken Window" (and a pretty big one too) is the 16 times people re-implemented the 'printUsage()' function (with exactly the same code) and, equally, added the `--help` flag (with exactly the same words) - this was done exactly following the principles you are advocating: namely, this is what was done elsewhere in the code, so it must be The Right Way of doing things. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34193/#review84181 ----------------------------------------------------------- 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: <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 > >