----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19416/#review37810 -----------------------------------------------------------
Thank you for pulling this out!! Looks great! 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19416/#comment69546> Now that it's out of the header file, can we pull 'cleanup' out of the 'process' namespace, or is there a reason to keep it in there? Continuing our discussion from the previous review: >> Anonymous namespaces are, as far as I know, new style in libprocess. >> Did you need this? > Need it? No. But it does make it so that the symbols aren't available > outside the translation unit. Ie, I can ensure that noone else is calling > cleanup/using Envp so I won't break people inadvertently if I change them. Which case are you concerned about? I don't see how people could be using our 'cleanup' unless they: -Forward declares a 'cleanup' with the same signature, and -Omit a definition for their 'cleanup' declaration. So they unknowingly get our version of cleanup at link-time, rather than a "multiple definition" linker error. But this seems pretty contrived so I'm not sure if it's the case you were thinking of? I think unnamed namespaces are a reasonably defensive tool. But I'd like to see explicit conversations with committers around new style (as Bernd did for 'explicit' constructors). Once agreed, it would be great to update our code to consistently follow the new style. And at the same time, we should try to strike a balance between high value changes for the project and technical debt cleanup. :) - Ben Mahler On March 19, 2014, 6:39 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19416/ > ----------------------------------------------------------- > > (Updated March 19, 2014, 6:39 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > see summary > > this is part zero of 19162 which will be rebased to depend on this. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76 > 3rdparty/libprocess/include/process/subprocess.hpp > 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6 > 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/19416/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
