> 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. - 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 > >
