mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 19416: Moved subprocess and cleanup method to new cpp.
Date Wed, 02 Apr 2014 21:57:19 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message