> On Sept. 12, 2014, 7:03 p.m., Ben Mahler wrote: > > Thanks for doing this! A few higher level comments: > > > > (1) We have strings::join in stout. Have you considered implementing this > > TLineHelper as a generic Joiner in stout/strings.hpp? Seems like it belongs > > there instead of a newline joiner in this file. Let's also be sure to not > > expose it in the higher level namespace. > > > > (2) We tend to leverage support/post-reviews.py to break changes apart, for > > example, the addition of the variadic template and the breaking up of help > > into a .cpp file could have been done in two separate reviews to speed up > > the review / commit process. We could get the latter change committed > > quickly! As much as possible, breaking changes apart into a chain of > > commits will make your life easier. :) > > > > (3) From the discussion on MESOS-750, it seems that gcc 4.4 is the minimum > > supported compiler version. FWICT, this means that within mesos we can > > certainly assume variadic templates. My hunch is that we don't want to > > continue writing both C++11/non-C++11 code paths in stout/libprocess due to > > the complexity and the maintenance burden, but that's a decision we should > > seek benh's input on. For now, the #ifdef is certainly a safe way to go.
(1) There are actually a number string joiners throughout the codebase, and lots of unnecessary constant string -> std::string -> appending. Things like the newline adder add a lot of extra compile time while not actually giving less typing than people inserting '\n' by hand (On top of the runtime behavior doing less unnecessary computation)... (3) I've worked with variadic templates on gcc 4.6 - 4.9, and each until about 4.7 has slight changes in behavior / bugfixes. We really need a bot moving into C++11 land to guarantee that all the compilers we say we support work. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25079/#review53198 ----------------------------------------------------------- On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25079/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2014, 4:24 p.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Bugs: MESOS-1734 > https://issues.apache.org/jira/browse/MESOS-1734 > > > Repository: mesos-git > > > Description > ------- > > Reduce compile time: - replacing a macro expansion with a variadic template > - moving implementation from help.hpp to help.cpp > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am edbe54b > 3rdparty/libprocess/include/process/help.hpp 4333b5b > 3rdparty/libprocess/src/help.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25079/diff/ > > > Testing > ------- > > Ran "make check". > > > Thanks, > > Patrick Reilly > >
