Bug 10828 – datetime toString functions should accept sink

Basically, all datetime functions' "toString" signature are: "string toString();" There should be an overload which accepts a sink (eg:) "const void toString(scope void delegate(const(char)[]) sink);" Without this, code such as: writeln(currentTime) Will needlessly allocate a string. This is an issue for this user: http://forum.dlang.org/thread/[email protected]#post-ypgppdursxtwljhkjoed:40forum.dlang.org Who wants to log the time at which destruction happens. This leads to a: core.exception.InvalidMemoryOperationError Due to an allocation during a collect. Implementation should be "trivial"*, since the implementation of "toString" is basically "return format("%%%%", args)". So this can "easily" be changed to: "formattedWrite(sink, "%%%%", args)"

The problem will move on to the checks of the format specifiers, which want to throw exceptions, which means the GC could in turn throw InvalidMemoryOperationError. Now one could fix the GC to allow exceptions to be thrown from ~this(), but basically if the dtor really fails, your program is possibly in an invalid state. Maybe my thinking is wrong, but I'd say it should either complete or terminate the application like AssertError, InvalidMemoryOperationError or OutOfMemoryError do. That said, the logging should be wrapped in a try-catch here: It is more important that the rest of the dtor succeeds. About the GC, I don't know what to do. It would be best if the implementation could allow further allocations while the collection is running. Like when you create a new pool for the allocations and join it with the old one after collection.

(In reply to comment #1) > The problem will move on to the checks of the format specifiers, which want to > throw exceptions, which means the GC could in turn throw > InvalidMemoryOperationError. An exception is only thrown if the format string is invalid. Furthermore, there *is* a toString which simply does not take format. So *that* should never throw (though it might not actually be nothrow, due to UTF). Besides, GC and destructors is only one aspect of the issue. Having a `toString` that takes a sink is just good design overall. Avoids gratuitous allocations.

Regardless of whether this fixes that particular user's problem, it's certainly the case that there should be to*String overloads in std.datetime which take sinks, because that's what we should be doing with all toString functions.

(In reply to Jonathan M Davis from comment #3) > Regardless of whether this fixes that particular user's problem, it's > certainly the case that there should be to*String overloads in std.datetime > which take sinks, because that's what we should be doing with all toString > functions. @jmdavis: Are you still working on the re-packaging of datetime? What's the status on that?

> @jmdavis: Are you still working on the re-packaging of datetime? What's the status on that? Enough has changed that I'm going to have to redo it, and I'm short on time these days. It's currently my plan to make a series of smaller pull requests to clean up the code further prior to splitting the module so that I don't have to do as much cleanup when splitting it.

I object to creating a same-only-different design instead of using Output Ranges.

But a sink *is* an output range!

(In reply to Walter Bright from comment #6) > I object to creating a same-only-different design instead of using Output > Ranges. That's the way it is designed though: http://dlang.org/phobos/std_format.html#.formatValue It's documented, does not allocate, supports unicode but does not decode, and is already rolled out on lots of data types, including (but not limited to) things like complex. complex even supports custom format flags. There's a wiki entry: http://wiki.dlang.org/Defining_custom_print_format_specifiers And I also believe there are stack overflow articles about how awesome this design is. Also, it's not "same-only-different" design, as end users shouldn't actually be calling "toString": This function is merely a hook for higher order functions like "format", "to!", "writeln" etc... So we'd really be doing: `string toString()` => `void toString(scope void delegate(const(char)[]) sink)` The old function would be replaced, so there'd be no duplicate function. At this point, objecting to this change is objecting to Phobos' entire user defined type to string mechanism.

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/1eeadc82d39a31db0d780413ded085e1edb1b359 Work On Issue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/23c960177068b7156d8efff7ff08011d24f63e6d Merge pull request #6060 from JackStouffer/date-tostring Partially Fix Issue 10828 - Add output range version of toString functions to std.datetime.Date merged-on-behalf-of: Jack Stouffer <[email protected]>

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/d5528a62c6319268d9b0e2f6f84bb27b520a7c3f Work On wqIssue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/dadb13da4678b887c33a697c269543590dc50872 Merge pull request #6071 from JackStouffer/timeofday-tostring Work On Issue 10828 - Add output range versions of toString functions to std.datetime.TimeOfDay merged-on-behalf-of: Jack Stouffer <[email protected]>

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/b503bb4acd8bd32ba95dbac29a04fda45af0eceb Work On Issue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/e03f97025240ecdcc5639531a68462a03daa893b Merge pull request #6101 from JackStouffer/datetime-tostring Issue 10828 - Add Output Range Overloads to DateTime toString Methods

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/a55744aa69158fb75d92753daf93210bf7f3548d Work On Issue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/3b3c3cb6be79140943405f94c009f23f8d109039 Merge pull request #6173 from JackStouffer/timezone-tostring Work On Issue 10828 - Add Writer Versions of TimeZone toStrings merged-on-behalf-of: Jonathan M Davis <[email protected]>

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/1334538b4d34a0c3d79f63b0b47aa05587564bd7 Work On Issue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/bc1b9569aa1ebb1713434c081c0171e206ae7215 Merge pull request #6180 from JackStouffer/systime-toisostring Work On Issue 10828 - Added writer version of SysTime.toISOString merged-on-behalf-of: Jack Stouffer <[email protected]>

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/dcdf8b4125669266067e4693e5ddc8ac746ed87b Fix Issue 10828 - datetime toString functions should accept sink https://github.com/dlang/phobos/commit/5ccd40cf4ecbe3d073789b4fb3a23e5710a9c520 Merge pull request #6190 from JackStouffer/systime-toISOExtString Fix Issue 10828 - datetime toString functions should accept sink merged-on-behalf-of: Jack Stouffer <[email protected]>