Re: [boost] [chrono][system] Make possible to don't provide hybrid error handling.

Subject: Re: [boost] [chrono][system] Make possible to don't provide hybrid error handling.
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-10-11 16:28:25

Le 11/10/11 19:48, Stewart, Robert a écrit :
> Vicente J. Botet Escriba wrote:
>> The library could add a macro that prevent the inclusion of
>> these prototype, as e.g.
>> BOOST_CHRONO_DONT_PROVIDE_HYBRID_ERROR_HANDLING.
> If you had to go that way, s/DONT_PROVIDE/NO/!
>
>> The experience while using these hybrid approach and mixing
>> all these semantics has shown me that this doesn't promotes
>> good design and doesn't compose well. Split the prototypes
>> is cleared.
>>
>>     system_clock::time_point system_clock::now() noexcept
>>     {
>>       timespec ts;
>>       if ( ::clock_gettime( CLOCK_REALTIME,&ts ) )
>>       {
>>         BOOST_ASSERT(0&&  "Boost::Chrono - Internal Error");
>>       }
>>       return time_point(duration(
>>         static_cast<system_clock::rep>( ts.tv_sec ) *
>>            1000000000 + ts.tv_nsec));
>>     }
>>
>>     system_clock::time_point
>>     system_clock::now(system::error_code&  ec)
>>     {
>>       timespec ts;
>>       if ( ::clock_gettime( CLOCK_REALTIME,&ts ) )
>>       {
>>           if (BOOST_CHRONO_IS_THROWS(ec)) //[1]
>>           {
>>               boost::throw_exception(
>>                       system::system_error(
>>                               errno,
>>                               BOOST_CHRONO_SYSTEM_CATEGORY,
>>                               "chrono::system_clock" ));
>>           }
>>           else
>>           {
>>               ec.assign( errno, BOOST_CHRONO_SYSTEM_CATEGORY );
>>               return time_point();
>>           }
>>       }
>>
>>       if (!BOOST_CHRONO_IS_THROWS(ec)) // [2]
>>       {
>>           ec.clear();
>>       }
>>       return time_point(duration(
>>         static_cast<system_clock::rep>( ts.tv_sec ) *
>>            1000000000 + ts.tv_nsec));
>>     }
>>
>>
>> Note how the test on whether the ec is thows() in [1] to throw
>> and in [2] see if the ec can be cleared are obscuring what in
>> principle was simple.
> [2] isn't needed.  In the case of no error, you clear ec.  In the case of an error, you have extra work to do and, frankly, don't care about efficiency.  Therefore:
>
> system_clock::time_point
> system_clock::now(system::error_code&  _code)
> {
>     _code.clear();
>     timespec ts;
>     if (::clock_gettime(CLOCK_REALTIME,&ts))
>     {
>        if (BOOST_CHRONO_IS_THROWS(_code))
>        {
>           boost::throw_exception(
>              system::system_error(
>                 errno, BOOST_CHRONO_SYSTEM_CATEGORY,
>                 "chrono::system_clock" ));
>        }
>        else
>        {
>           _code.assign(errno, BOOST_CHRONO_SYSTEM_CATEGORY);
>           return time_point();
>        }
>     }
>     return time_point(
>        duration(static_cast<system_clock::rep>(ts.tv_sec)
>           * 1000000000 + ts.tv_nsec));
> }
>
> There's still logic to check for the throws() case, but the result is a bit cleaner.
I don't remenber if boost:throws() returned or returns a reference to 0, 
so I nneded to check before writing. Beman?
>
>> It will be maybe better to add yet a 3rd prototype that forces
>> the throw_on_error.
>>
>> Clock::now() noexcept;
> 20.9.5 doesn't specify noexcept for now().  Should you add that?
See Beman's response.
>
>> Clock::now(system::error_code&) noexcept;
>> Clock::now(throw_on_error_t&);
> If you don't make the no-arguments overload be noexcept, then you get this:
>
> Clock::now();
> Clock::now(system::error_code&) noexcept;
>
> The first can throw; the second doesn't.  That, of course, could eliminate the throws() usage.
I will prefer to provide at least the standard inteface. So

   Clock::now() noexcept;

is mandatory for me.

>
>>     system_clock::time_point system_clock::now()
>>        BOOST_CHRONO_NOEXCEPT
>>     {
>>       timespec ts;
>>       if ( ::clock_gettime( CLOCK_REALTIME,&ts ) )
>>       {
>>         BOOST_ASSERT(0&&  "Boost::Chrono - Internal Error");
>>       }
>>       return time_point(duration(
>>         static_cast<system_clock::rep>( ts.tv_sec ) *
>>            1000000000 + ts.tv_nsec));
>>     }
>>
>>     system_clock::time_point
>>     system_clock::now(system::error_code&  ec)
>>     {
>>       timespec ts;
>>       if ( ::clock_gettime( CLOCK_REALTIME,&ts ) )
>>       {
>>           ec.assign( errno, BOOST_CHRONO_SYSTEM_CATEGORY );
>>           return time_point();
>>       }
>>
>>       ec.clear();
>>       return time_point(duration(
>>         static_cast<system_clock::rep>( ts.tv_sec ) *
>>            1000000000 + ts.tv_nsec));
>>     }
>>
>>     system_clock::time_point system_clock::now(
>>        throw_on_error_t&  /*tag*/) noexcept
> Why by reference, anyway?
This was a type. No need for reference, of course.
>>     {
>>       timespec ts;
>>       if ( ::clock_gettime( CLOCK_REALTIME,&ts ) )
>>       {
>>               boost::throw_exception(
>>                       system::system_error(
>>                               errno,
>>                               BOOST_CHRONO_SYSTEM_CATEGORY,
>>                               "chrono::system_clock" ));
>>       }
>>       return time_point(duration(
>>         static_cast<system_clock::rep>( ts.tv_sec ) *
>>            1000000000 + ts.tv_nsec));
>>     }
> With my version, you'd get this instead:
>
> system_clock::time_point
> system_clock::now()
> {
>     system::error_code error;
>     time_point const result(now(error));
>     if (error)
>     {
>        boost::throw_exception(
>           system::system_error(error, "chrono::system_clock"));
>     }
>     return result;
> }
>
> system_clock::time_point
> system_clock::now(system::error_code&  _code) noexcept
> {
>     _code.clear();
>     timespec ts;
>     if (::clock_gettime(CLOCK_REALTIME,&ts))
>     {
>        _code.assign(errno, BOOST_CHRONO_SYSTEM_CATEGORY);
>        return time_point();
>     }
>     return time_point(
>        duration(static_cast<system_clock::rep>(ts.tv_sec)
>           * 1000000000 + ts.tv_nsec));
> }
>
> That seems very clear.  There's no duplication.
See my comment above.
>
>> I see three alternatives:
>>
>> A- Standard
>> Clock::now() noexcept;
>>
>> B- Standard+Hybrid
>> Clock::now() noexcept;
>> Clock::now(system::error_code&); // takes care of
>> boost::throw()
>>
>> C-Standard+EC+TOE
>> Clock::now() noexcept;
>> Clock::now(system::error_code&) noexcept;
>> Clock::now(throw_on_error_t&);
>>
>> A breaks code using the error_code.
>> C breaks code using the boost::throws()
>>
>> Yet another temporary alternative:
>> D-
>> Clock::now() noexcept;
>> Clock::now(system::error_code&); // takes care of
>>     boost::throw() but it is deprecated
>> Clock::now(throw_on_error_t&);
> Now E-
>
> Clock::now();
> Clock::now(system::error_code&);
>
>> We can start by providing D and let the user using
>> boost::throws to move to throw_on_error, and the move to C.
>> In addition the library must provide the user the possibility
>> to don't include the extensions.
> With E, there are fewer overloads and the semantics are easy to explain.  The downside is current uses of boost::throws() will break.  Nevertheless, that avoids any deprecation hassles as the change forces updating calling code to match the change in semantics.
>
>
The idea of Beman was to reduce to a single interface

Clock::now(system::error_code&=boost::throws());

But it was not accepted and with noexcept, you can not have all in a single prototype.

What others think about whether Boost.Chono must provide at least the 
standard interface?

Best,
Vicente