On Wed, Apr 2, 2014 at 2:57 PM, Benjamin Hindman <[email protected]> wrote:
> > > > On March 19, 2014, 10:15 p.m., Ben Mahler wrote: > > > 3rdparty/libprocess/src/subprocess.cpp, line 13 > > > < > https://reviews.apache.org/r/19416/diff/1/?file=528343#file528343line13> > > > > > > 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. :) > > > > Dominic Hamon wrote: > > 1. There's no reason to keep cleanup in process, except that it is > being compiled as part of process and is visible as an external symbol to > other translation units unless it's in a... > > > > 2. unnamed namespace. This is less about accidental use and more > about someone finding cleanup and trying to use it. I agree that's unlikely > in this case, but there are other cases where it is useful and it's become > a habit of mine to use it whenever I'm writing a helper method or class in > a translation unit that I don't want visible outside that translation unit. > > > > We should talk about these more, but it's not exactly high priority. > I'll remove it from this CL. > > > > > > Ben Mahler wrote: > > Sounds good! > > For what it's worth, while we haven't used anonymous namespaces we have > used 'static' some places to limit exposure. > There was a time when static was deprecated for that, though it seems to have come back. Un-named namespaces are still useful for defining types that are local to a translation unit. But yes, either works. The former is just my muscle memory still twitching. > > > - Benjamin > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19416/#review37810 > ----------------------------------------------------------- > > > On March 20, 2014, 10:58 p.m., Dominic Hamon wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/19416/ > > ----------------------------------------------------------- > > > > (Updated March 20, 2014, 10:58 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 > > > > > >
