try/catch idiom in std.datetime

[Andrei Alexandrescu]

On 11/17/13 11:28 PM, Jonathan M Davis wrote:
>> 3. Look into API changes that add nothrow, narrower functions to the
>> more general ones that may throw.
>
> In some cases (e.g. format) that's probably a good idea, but I don't think
> that really scales. In some cases, it would be well worth it, whereas in
> others, it would just be better to use try-catch in the few places where you
> know the function won't throw, because it would be quite rare to be able to
> guarantee that it wouldn't throw. It's also not pleasant to have to duplicate
> functions all over the place.

I thought about this some more. So std.datetime accounts for > 1/3 of 
all try statements in Phobos. (It also holds a number of other odd 
records - talking from allegations: largest module in source code, 
requires most memory to build for unittest, most unittest lines per line 
of working code. It's great that these are being worked on.) That must 
mean something. The question is what. It may as well mean that the rest 
of Phobos is sloppy and does not add nothrow in places where it should, 
which would make the rest of it insert a bunch of try/catch in the same 
pattern as std.datetime. An argument based on sense and sensibility 
would suggest that something ought to be done about that idiom in 
std.datetime. But the argument "the more nothrow the merrier" is also 
sensible.

So I think a closer look is warranted here, on a case basis. If the 
conclusion is that such inspection doesn't scale, fine, we've learned 
something. But I think there are more valid learnings to derive from here.

I'll walk through a few samples I gathered below, please don't get 
offended (I know it can feel odd to have one's own code criticized). All 
in good spirit.

1. Consider:

     this(in DateTime dateTime, immutable TimeZone tz = null) nothrow
     {
         try
             this(dateTime, FracSec.from!"hnsecs"(0), tz);
         catch(Exception e)
             assert(0, "FracSec's constructor threw when it shouldn't 
have.");
     }

That's because FracSec.from!"hnsecs"(n) may throw for some values of n. 
To me, this is overkill. Clearly there must be a way to construct a zero 
time interval without much code being executed at all, let alone checks 
and exceptions and whatnot. (In particular FracSec.zero looks like the 
ticket.) Furthermore, having a simpler constructor call a more 
complicated constructor reminds one of the mathematician who throws away 
the water in the kettle to regress to an already solved problem.

2. Consider:

     this(in DateTime dateTime, in FracSec fracSec, immutable TimeZone 
tz = null)
     {
         immutable fracHNSecs = fracSec.hnsecs;
         enforce(fracHNSecs >= 0, new DateTimeException("A SysTime 
cannot have negative fractional seconds."));
         _timezone = tz is null ? LocalTime() : tz;

         try
         {
             immutable dateDiff = (dateTime.date - Date(1, 1, 
1)).total!"hnsecs";
             immutable todDiff = (dateTime.timeOfDay - TimeOfDay(0, 0, 
0)).total!"hnsecs";

             immutable adjustedTime = dateDiff + todDiff + fracHNSecs;
             immutable standardTime = _timezone.tzToUTC(adjustedTime);

             this(standardTime, _timezone);
         }
         catch(Exception e)
         {
             assert(0, "Date, TimeOfDay, or DateTime's constructor threw 
when " ~
                       "it shouldn't have.");
         }
     }

This is not even marked as nothrow (sic!) so the entire try/catch is 
completely meaningless - probably a rote application of the idiom 
without minding its intent. The only difference it would make to the 
user is that a library bug may manifest in slighlty different ways. The 
code has a smartaleck thing about it: "You'd be surprised to see an 
exception here. I, too, would be surprised, and I wanted to make sure I 
tell you about it". The entire try/catch should be removed.

3. Consider:

     this(in Date date, immutable TimeZone tz = null) nothrow
     {
         _timezone = tz is null ? LocalTime() : tz;

         try
         {
             immutable adjustedTime = (date - Date(1, 1, 1)).total!"hnsecs";
             immutable standardTime = _timezone.tzToUTC(adjustedTime);

             this(standardTime, _timezone);
         }
         catch(Exception e)
             assert(0, "Date's constructor through when it shouldn't 
have.");
     }

Here, again, we have a nothrow constructor call a more general one, 
which may throw in general. The intent - maximizing internal reuse 
through forwarding - is noble. The realization - it takes more code to 
reuse than to just set the blessed variables - is a corruption of the 
noble goal.

4. Consider:

     @property FracSec fracSec() const nothrow
     {
         try
         {
             auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);

             if(hnsecs < 0)
                 hnsecs += convert!("hours", "hnsecs")(24);

             hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);

             return FracSec.from!"hnsecs"(cast(int)hnsecs);
         }
         catch(Exception e)
             assert(0, "FracSec.from!\"hnsecs\"() threw.");
     }

This is an interesting one. Going to FracSec.from's documentation, we 
see that it throws if the input does not fall within (-1, 1) seconds, 
which means (if I count correctly) hnsec inputs must fall between -10 
million and 10 million. But we already know that we're in good shape 
because we just called removeUnitsFromHNSecs. The problem is the two 
calls are disconnected. That means the API could be improved here: 
combine the two functions by defining a way to get the fractionary 
FracSec from a time value. That will always succeed by definition, so 
we're in good shape.

5. Consider:

     tm toTM() const nothrow
     {
         try
         {
             auto dateTime = cast(DateTime)this;
             tm timeInfo;

             timeInfo.tm_sec = dateTime.second;
             timeInfo.tm_min = dateTime.minute;
             timeInfo.tm_hour = dateTime.hour;
             timeInfo.tm_mday = dateTime.day;
             timeInfo.tm_mon = dateTime.month - 1;
             timeInfo.tm_year = dateTime.year - 1900;
             timeInfo.tm_wday = dateTime.dayOfWeek;
             timeInfo.tm_yday = dateTime.dayOfYear - 1;
             timeInfo.tm_isdst = _timezone.dstInEffect(_stdTime);

             version(Posix)
             {
                 char[] zone = (timeInfo.tm_isdst ? _timezone.dstName : 
_timezone.stdName).dup;
                 zone ~= "\0";

                 timeInfo.tm_gmtoff = cast(int)convert!("hnsecs", 
"seconds")(adjTime - _stdTime);
                 timeInfo.tm_zone = zone.ptr;
             }

             return timeInfo;
         }
         catch(Exception e)
             assert(0, "Either DateTime's constructor threw.");
     }

In fact the assertion message is wrong here. There's no constructor that 
could fail. The problem is with the .dup! That's a bug in .dup for 
char[] - it may indeed throw, but that's an Error not an exception.

(Anyhow, I got curious about tm_zone out of concern for the odd 
allocation. According to 
http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_19.html it's 
a gnu extension and it's a const char*, meaning the user code is not 
supposed to mess with it. Therefore, my guess is some sort of 
memoization here would be in order. But then 
http://www.gsp.com/cgi-bin/man.cgi?topic=mktime suggests it's not const. 
Then 
http://www.eecs.harvard.edu/syrah/vino/release-0.40/man/programref/libc/time/ctime.3.txt 
says "he tm_zone field of a returned struct tm points to a static array 
of characters, which will also be overwritten at the next call (and by 
calls to tzset)." That means a static char[3] would be in order. Anyhow, 
more investigation is needed here.)

6. Consider:

     /+ref SysTime+/ void roll(string units)(long value) nothrow
         if(units == "hours" ||
            units == "minutes" ||
            units == "seconds")
     {
         try
         {
             auto hnsecs = adjTime;
             auto days = splitUnitsFromHNSecs!"days"(hnsecs) + 1;

             if(hnsecs < 0)
             {
                 hnsecs += convert!("hours", "hnsecs")(24);
                 --days;
             }

             immutable hour = splitUnitsFromHNSecs!"hours"(hnsecs);
             immutable minute = splitUnitsFromHNSecs!"minutes"(hnsecs);
             immutable second = splitUnitsFromHNSecs!"seconds"(hnsecs);

             auto dateTime = DateTime(Date(cast(int)days), 
TimeOfDay(cast(int)hour, cast(int)minute, cast(int)second));
             dateTime.roll!units(value);
             --days;

             hnsecs += convert!("hours", "hnsecs")(dateTime.hour);
             hnsecs += convert!("minutes", "hnsecs")(dateTime.minute);
             hnsecs += convert!("seconds", "hnsecs")(dateTime.second);

             if(days < 0)
             {
                 hnsecs -= convert!("hours", "hnsecs")(24);
                 ++days;
             }

             immutable newDaysHNSecs = convert!("days", "hnsecs")(days);

             adjTime = newDaysHNSecs + hnsecs;
         }
         catch(Exception e)
             assert(0, "Either DateTime's constructor or TimeOfDay's 
constructor threw.");
     }

Here again the error message is mistaken; the only culprit is 
TimeOfDay's constructor. This prompts a different protest. Note that up 
until the point liable to throw, the input "value" is not involved at 
all, meaning that whatever simple manipulation is done there cannot be 
done without an inefficiency being in the loop. So there's a problem 
with the API design.

7. Consider:

     DateTime opCast(T)() const nothrow
         if(is(Unqual!T == DateTime))
     {
         try
         {
             auto hnsecs = adjTime;
             auto days = splitUnitsFromHNSecs!"days"(hnsecs) + 1;

             if(hnsecs < 0)
             {
                 hnsecs += convert!("hours", "hnsecs")(24);
                 --days;
             }

             immutable hour = splitUnitsFromHNSecs!"hours"(hnsecs);
             immutable minute = splitUnitsFromHNSecs!"minutes"(hnsecs);
             immutable second = getUnitsFromHNSecs!"seconds"(hnsecs);

             return DateTime(Date(cast(int)days), 
TimeOfDay(cast(int)hour, cast(int)minute, cast(int)second));
         }
         catch(Exception e)
             assert(0, "Either DateTime's constructor or TimeOfDay's 
constructor threw.");
     }

This seems to be another instance of the problem at (6). No matter how 
one looks at it, one must admit that the API should be designed such 
that a valid SysTime should convert to the corresponding DateTime 
without much intervening brouhaha.

=============

That 7 issues with as many try/catch instances, i.e. 13% of the total 
54. You may think I cherry-picked them; in fact, they are the FIRST 7 
instances I found by simply searching the file for 'try'. I have no 
reason to believe I won't find similar issues with most or all of them.

I think std.datetime needs a pass with an eye for this idiom and 
obviating it wherever possible.


Andrei