Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow

Subject: Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Denis Shevchenko (for.dshevchenko_at_[hidden])
Date: 2011-02-08 02:50:59

My 2 cents, about source code...

1. In class boost::process::child.

      There is a type 'std::map<stream_id, handle>' that used several 
times. May be typedef?

2. In config.hpp file.

     Macros will be easier for maintenance if align backlslashes.
     Compare:

     #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \
     boost::throw_exception(boost::system::system_error( \
         boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \
             boost::system::get_system_category()), \
         BOOST_PROCESS_SOURCE_LOCATION what))

     and

     #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what)         \
     boost::throw_exception(boost::system::system_error( 
                         \
         boost::system::error_code(BOOST_PROCESS_LAST_ERROR,            \
             boost::system::get_system_category()),                     
                   \
         BOOST_PROCESS_SOURCE_LOCATION what))

3. In constructor of class boost::process::context.

     In this place you insert a values into map, but not search them.
     So use insert() instead of operator[]. Of course, with 
boost::assign ;-)

     using namespace boost::assign;
     insert( streams )( stdin_id,    behavior::inherit(STDIN_FILENO) )
                               ( stdout_id, 
behavior::inherit(STDOUT_FILENO) )
                               ( stderr_id,  
behavior::inherit(STDERR_FILENO) )
                               ;

4. In some classes you use public inheritance from boost::noncopyable, 
but you can use private inheritance instead.

5. In constructors of some classes you forgot 'explicit'.

6. In function boost::process::self::get_environment().

         environment e;
          // ...
         try
         {
             char *env = ms_environ;
             while (*env)
             {
                 std::string s = env;
                 std::string::size_type pos = s.find('=');
                 e.insert(environment::value_type(s.substr(0, pos),
                     s.substr(pos + 1)));
                 env += s.size() + 1;
             }
         }
         catch (...)
         {
             // ...
         }

         And what kind of exceptions can occur here?
             std::string::find,
             std::string::substr,
             std::map::insert,
             std::string::operator+=
             std::string::size
         these functions do not throw exceptions...

7. In class boost::process::detail::basic_status_service.

     There is a type 'boost::unique_lock<boost::mutex>' that used many 
times. May be typedef?

8. In some places of code you using std::algorithms. You can use 
boost::range::algorithms instead, for simplicity.

     Compare:

     std::find(impls_.begin(), impls_.end(), impl);

     and:

     boost::range::find( impls_, impl );

9. You can replace some 'native' cycles by BOOST_FOREACH.

10. You can initialize map easier:

     Compare:

     std::map<stream_id, handle> parent_ends;
     parent_ends[stdin_id] = handles[stdin_id].parent;
     parent_ends[stdout_id] = handles[stdout_id].parent;
     parent_ends[stderr_id] = handles[stderr_id].parent;

     and:

     using namespace boost::assign;
     std::map<stream_id, handle> parent_ends = map_list_of( stdin_id,  
handles[stdin_id].parent )
                                                          ( stdout_id, 
handles[stdout_id].parent )
                                                          ( stderr_id, 
handles[stderr_id].parent )
                                                          ;

11. In many places of code you using std::map. May be better to use 
boost::unordered_map, for optimizing the speed of searching?

Best regards, Denis.