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