> On March 19, 2014, 3: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. :)
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. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19416/#review37810 ----------------------------------------------------------- On March 19, 2014, 3:25 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, 3:25 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 > >
