Re: [boost] [process] A doubt about private constructors

From: Johan Nilsson (r.johan.nilsson_at_[hidden])
Date: 2006-08-25 02:03:12

Julio M. Merino Vidal wrote:
> Hello everybody,
>
> At the moment, some objects in the preliminary Boost.Process library
> are only constructed internally by the library.  Examples of these
> include 'status' instances, created by calls to 'child::wait', and
> instances of 'child', created by 'launcher::start'.  This is currently
> done by declaring friendship among the related classes (well, not
> exactly, but almost).
>
> I believe this is suboptimal because it prevents extensibility.
> Consider a user that wants to create his own Launcher implementation.
> His code is required to return a new 'child' object.  But, oh!  He
> cannot currently create it because the constructor is private and his
> launcher class is not a friend of the provided 'child'.  Of course, he
> could implement his own 'child' -- and in turn implement his own
> 'status'.  Ouch.

I was looking at the code below. Here the ctors are actually protected, 
which makes them public in practice - just derive a new status class and off 
you go. OTOH, if you use the status class by value it may be subject to 
slicing anyway. Or is the "protected" just a simple typo?

>
> In order to solve this, I was now adding some free functions that call
> the appropriate constructors.  This way, the class' constructor is
> kept private to avoid "accidental" creations from user code, but the
> user still has the ability to create such objects when he really wants
> to.  For example:
>
> class status {
> protected:
>    status(int flags);
>    friend status create_status(int flags);
> ...
> };
>
> inline
> status
> create_status(int flags)
> {
>    return status(flags);
> }
>
> The first thing I'm wondering is if this design approach makes sense
> or I should simply make the constructor public.  No matter the answer,
> the following still applies.
>
> But then, there is a worse problem (which made me write this mail in
> the first place).  Consider the child's constructor:
>
> class child {
> protected:
>    child(handle_type h,
>          detail::file_handle fhstdin,
>          detail::file_handle fhstdout,
>          detail::file_handle fhstderr);
>    friend child create_child(handle_type h,
>                              detail::file_handle fhstdin,
>                              detail::file_handle fhstdout,
>                              detail::file_handle fhstderr);
> };
>
> So far so good: the create_child function can now be used by the
> user... except that... he now has to deal with the file_handle class
> which was kept completely private until now.  So a couple of questions
> arise:
>
> 1) Should the user be allowed to construct these classes (for the
>   reasons explained above)?

As I wrote earlier - it is already possible if the code above is correct.

>
> 2) If 1 is "yes", then file_handle (and any other class that appears
>   in other create_* functions) needs to be made public.  This exposes
>   some classes that are not really about "process management" to the
>   user, but maybe that is not a problem?  Eventually these classes
>   may end up being part of some other library that deals with
> low-level OS stuff (Boost.System?).

Having the file_handle class public helps when integrating with legacy 
and/or platform-specific code. I haven't read all the docs, but I think 
exposing both the process id and file_handle as e.g. native_process_handle 
and native_file_handle would be a good thing. Note that this isn't about 
allowing users to actually create e.g. an instance of 'child' -> just about 
exposing the handles. Oh, and move them out of "detail", of course.

You can't possibly wrap all O/S functionality portably in your library, so 
providing a way for the end user to at least access these values helps them 
to use the portable code when possible, but still resort to non-portable 
calls when there's no other way. For an example, under Win32 you'd perhaps 
like to wait for a (Timer to expire _or_ data being available from the 
childs stdout) - it would be much cleaner to be able to use the native 
stdout handle in a call to WaitForMultipleObjects than some other approach.

Let the user write non-portable code when they _must_, while retaining the 
possibility to take advantage of your library. IIRC there's precedence in 
Boost.Asio.

Just my 0.02EUR.

// Johan