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
