Bug 12828 – Fix SimpleTimeZone.utcOffset so that it has the correct return type
Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2014-05-31T20:13:00Z
Last change time
2015-06-09T05:15:24Z
Assigned to
nobody
Creator
issues.dlang
Comments
Comment #0 by issues.dlang — 2014-05-31T20:13:49Z
It's the policy of time-related code in druntime and Phobos to never use naked integral values for time - in particular, any duration of time should use core.time.Duration rather than an integer for seconds or microseconds or whatnot.
I got this right almost everywhere in core.time and std.datetime, but for some reason I screwed up when writing SimpleTimeZone and didn't catch it before it got merged into Phobos. SimpleTimeZone.utcOffset is supposed to return the offset from UTC for that time zone, and should return a Duration. Unfortunately, it returns an int which represents minutes (something which is done with the POSIX functions in glibc and whatnot). So, utcOffset needs to be changed to be a Duration.
Comment #1 by issues.dlang — 2014-05-31T20:24:18Z
I fixed the setter for utcOffset some time ago but didn't fix the getter, because if I did, it would break code (since the return type changed). I've been holding out for multiple alias thises in the hopes of declaring something like
@property auto utcOffset() @safe const pure nothrow
{
static struct Result
{
Duration utcOffset;
int toInt() @safe const pure nothrow
{
return cast(int)utcOffset.total!"minutes"();
}
alias utcOffset this;
deprecated("utcOffset is being change to a Duration. " ~
"Please use it as one rather than int. " ~
"See documentation for details.")
alias toInt this;
}
return Result(_utcOffset);
}
and then change it to Duration later. However, there is still no sign of multiple alias thises being implemented, and I'm not sure that they ever will be. Also, given that we have core.time.minutes as well as core.time.Duration.minutes, depending on how multiple aliases thises was implemented, something like
auto foo = stz.utcOffset.hours;
could result in a compilation error, meaning that we still might break code with that attempt at a deprecation cycle.
So, what I'm now proposing to do is to simply change it. This _is_ unfortunately a breaking change, but it's _very_ easy to fix, and it's likely that it's relatively rare for anyone to use SimpleTimeZone.utcOffset. Something like
auto foo = stz.utcOffset;
would have to be changed to
auto foo = cast(int)stz.utcOffset.total!"minutes"();
So, we'd be making std.datetime properly consistent (since everywhere else that we have utcOffset, it's a Duration) and getting rid of the problem of having a naked integral value for time. I normally wouldn't go for this, as it does cause breakage (albeit breakage which is very easy to fix), but I don't see any other way to fix this design flaw at this point, and it _is_ easy to fix.