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: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-10-11 13:48:47

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.

> 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?

> 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.

>    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?

>    {
>      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.

> 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.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.