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.
Comment #2 by issues.dlang — 2014-05-31T20:38:46Z
Comment #3 by github-bugzilla — 2014-06-01T20:03:56Z
Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/47eb71f40a573536e0578e2c8cffb7bf9a73cd4c Fix issue 12828: Fix return type of SimpleTimeZone.utcOffset. https://github.com/D-Programming-Language/phobos/commit/24c4e8f600d15a5ac77ec85dbfa0001743491fb4 Merge pull request #2219 from jmdavis/datetime Fix issue 12828: Fix return type of SimpleTimeZone.utcOffset.