Re: [boost] [boost.process] Review

Subject: Re: [boost] [boost.process] Review
From: Giel van Schijndel (Giel.vanSchijndel_at_[hidden])
Date: 2016-09-12 08:41:08

Hi Klemens,

On 11/09/16 23:11, Klemens Morgenstern wrote:
> Boost.Process 0.6, as can be found here
> (https://github.com/klemens-morgenstern/boost-process) will be in
> review from October 27 to November 5. The review manager will be
> Antony Polukhin.
>
> Doc (http://klemens-morgenstern.github.io/process/)
>
> I really hope this library gets through the review, since boost (or
> C++ for that matter) really needs a process library. I would be really
> happy, if some of you could take the time to look into it - if you
> find any major issues, they might be addressed, before the actualy
> review. That would then increase the probability of the library
> getting accepted.

Just a few comments based on some quick observations:

 * You assume the presence of $PATH and a current working directory.
Unfortunately not all operating systems have these concepts: notably
Windows CE has neither environment variables nor a current working
directory.
 * Additionally current working directory duplicates functionality from
Boost.Filesystem
 * You seem to always take paths (on Windows) by 'char' based strings.
This will be problematic in that the Windows ("ANSI") API interprets
these dependent on the current code page. Providing the ability to use
'wchar_t' based strings (or just boost::filesystem::path) should
alleviate that problem because it's always UTF-16. Additionally, Windows
CE, in the incarnations that I've seen, doesn't even provide the "ANSI"
API, meaning the Unicode API is the only option.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel | Software Engineer | Millau | PU CS | TomTom Eindhoven