> 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. > > Cody Maloney wrote: > (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. > > Ben Mahler wrote: > (1) Good point, we can make strings::join variadic for C++11 and it can > do the job for us in help.hpp, to avoid further proliferation of string > joiners. :) > > (3) Ok, let's keep the #ifdef for now!
I have submitted r25789 and will modify this patch to use the strings::join provided from there. - Joris ----------------------------------------------------------- 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 > >
