Re: [boost] [date_time] [#1861] Change for the default duration format

Subject: Re: [boost] [date_time] [#1861] Change for the default duration format
From: Ilya Bobir (ilya.bobir_at_[hidden])
Date: 2009-06-16 16:53:48

Stewart, Robert wrote:
> Andrey Semashev wrote:
> On Monday, June 15, 2009 5:24 PM
>> On Monday 15 June 2009 19:42:18 Stewart, Robert wrote:
>>
>>>> 2. An exception is not a good choice to highlight the problem,
>>>> since in most cases it will be swallowed by IO streams. In
>>>> practice streaming errors (in form of failbit or badbit) are
>>>> often ignored, and Boost.DateTime is an example. And
>>>> ios_base::failure usually isn't as descriptive as what is being
>>>> initially thrown.
>>> Sorry, but no.  The default is for IOStreams to set the
>>> stream state to indicate failures, but that doesn't absorb
>>> exceptions from code such as we're discussing and convert
>>> exceptions into stream state.
>> It does adsorb exceptions, at least on my GCC 4.3.2. But it
>> does pass through original exceptions if exceptions are
>> enabled, but I'm not sure it's guaranteed by the standard. You
>> can play with the attached code sample to test your platform.
> 
> I went back and reread 27.6.1.2.1.  Despite my confident assertion above, there is no distinction made as to the source of exceptions caught.  It says, "if an exception is thrown during input then ios::badbit is turned on in *this's error state."  I'm sorry for the noise.
> 
> Throwing an exception, however, does mean that it will propagate from a stream when configured to permit that.  Setting badbit is the best that can be done otherwise and seems necessary, even if it is ignored, as it will prevent further operations on the current stream until cleared.  That may draw attention to the code using that stream as the result will deviate from the expected.  Obviously, badbit can be set without throwing an exception, but the chance for the exception to propagate, with the side effect of setting badbit, means throwing one is worthwhile.
> 
>>>> 3. The caller has to check the arguments. It's the bottom line
>>>> of both STL and most C functions. Extensive error checking may
>>>> be provided but it should be optional. An assert fits
>>>> perfectly.
>>> A well designed library prevents misuse.
>> Right. But since DateTime IO is based on facets, there isn't
>> much room for design decisions. As for the exception, it does
>> not prevent misuse. It *may* show when the feature *have been*
>> misused.
> 
> Since there's little room for design decisions, why not avail ourselves of all options at our disposal?  If throwing an exception doesn't reveal the misuse because exceptions are absorbed, badbit is ignored, and the incorrect result isn't noticed, then there's nothing to be done.  However, exceptions may be enabled, badbit may be noticed, and the incorrect result may be noticed, so throwing an exception increases the chance of detecting the misuse.
> 

General
-------

What is so different about

   time_duration td;
   cin >> td;

from

   int v;
   cin >> v;

that makes you discuss an alternative error reporting strategy for the 
time duration input facet?

Maybe I'm missing something but it seems to me that the original 
question was a bit different.  I'll try to explain the situation again.

There was a bug in the time duration output facet.  The facet used 
strftime to do output for values larger than 24 hours.  According to ISO 
9899-1999 7.23.3.5 par. 3 this is an undefined behavior.  This bug was 
discussed here:

http://lists.boost.org/Archives/boost/2007/12/131541.php

And the part of my patch that addressed this bug is already in the 
library as of 1.39.0.  The output for %H is now done without strftime.

As %H is used in other format strings besides time duration's one it was 
decided that a new specifier should be added that will allow input and 
output of an hours field that is longer than two digits.  This new 
specified is %O.

I doubt there is any use for %O in other formats except time duration's 
one as nor ptime, nor time period have any semantic for an hours field 
that is larger than 23.  On the other hand there is nothing special 
about a time duration of 23 or 24 hours.

There is a distinction between processing of %H in a time duration input 
facet and a time duration output facet.  I'll discuss them separately. 
The same code is used by input and output facets for all date_time 
objects.  While the format strings may be different for time duration 
and ptime the %H in these strings will be read identically.

%H in an output facet
---------------------
The hours value is converted into a string.  There is a BOOST_ASSERT 
that checks that the value is no longer than 2 characters.  At the same 
time a zero is perpended if the value is shorter than 2 characters.

Note that there is no check for the value of 23 or 24.  Just for the 
length of 2 characters.

A time duration that is 100 hours or longer will assert in a debug 
build, but will output correctly on a release one.

%H in an output facet
---------------------
A continuous sequence of digits no longer than 2 is read from a stream. 
  If there is less than 2 digits it is an error.  Read digits are 
converted into a number.  Actually it means that the current behavior is 
wrong with regard to the ptime or time period.  But I do not care about 
them so I leave the fix to someone else.  Or will fix it later.  See an 
example at the bottom of the email.

For time duration it means that it is OK to read any time duration that 
is no longer than 99 hour 99 minutes and 99 seconds if you do not change 
the time duration input format.

My proposal
-----------
My patch contained a modification to the default *time duration* format 
string.  It does not affect nor ptime, nor time period input format 
strings. I think that it makes sense to replace %H with %O in the 
default time duration format to make it possible to input and output 
arbitrary time durations.

There is an opinion that this is "a breaking change" and this part of 
the patch should be discussed on the mailing list:

https://svn.boost.org/trac/boost/ticket/1861#comment:3

Actually I see this as a bug fix.  But if someone really thinks that 
this change may break his or her code it would be nice to hear about it. 
  And probably I will try to find a better fix for this bug.

%H in an input facet for a ptime bug
------------------------------------
The following code

#include <iostream>
#include <sstream>

#include <boost/date_time.hpp>

using namespace std;
using namespace boost::posix_time;

int main(void)
{
     time_input_facet* facet = new time_input_facet();
     locale loc(locale(locale(), facet));

     istringstream ss("2009-Jun-16 28:14:00");
     ss.imbue(loc);

     /* Input using default time format. */

     facet->format("%Y-%b-%d %H:%M:%S%F %z");

     ptime p;
     ss >> p;

     cout << p << endl
         << "fail is " << boolalpha << ss.fail() << endl;
}

Gives me this output on MS cl:

2009-Jun-17 04:14:00
fail is false

While it would have failed to parse the input value, in my opinion.

-- 
Ilya Bobir