[RFC] Make Output Ranges Using Methods The Preferred toString Option #5991

JackStouffer · 2018-01-03T19:57:54Z

This proposal

Modifies std.format to recognize any toString that accepts a character accepting output range as having a valid toString overload
Modifies std.format to prioritize and use the output range method when formatting aggregate types if it exists
De-emphasizes the scope delegate toString methods in the docs

Rationale

Firstly, speed. According to Walter, the delegate passed as a parameter can never be in-lined because of how DMD does data flow analysis. Using instead the defined put method of an output range would solve that problem.

Secondly, the vision of Phobos. Ranges are the present and the future. Output ranges really need more support in Phobos (std.conv).

Thirdly, safety. The vast majority of the code I've seen doesn't mark the delegate as @safe to allow non-safe sinks to work with it. This means that the toString method itself can never be marked @safe either. Making toString a template who's safety relies on the put method of the output range side-steps this problem entirely.

dlang-bot · 2018-01-03T19:57:54Z

Thanks for your pull request, @JackStouffer!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

JackStouffer · 2018-01-03T20:48:19Z

Ho man, this is a fun bug. Turns out, if you don't define your FormatSpec as const ref in the parameters of your toString (and why would you when the docs don't tell you to do that), it wasn't recognized as a valid overload by formatObject before this change.

Case in point, BigInt's toString(scope void delegate(const(char)[]) sink, ref FormatSpec!char f) overload wasn't ever called by std.format. It called the string format option every time.

Fix incoming.

See #5991 (comment) for more information

JackStouffer · 2018-01-03T21:06:21Z

#5992

quickfur · 2018-01-03T22:06:33Z

std/format.d

+ * ---
+ *
+ * Where `W` allows a character accepting
+ * $(REF_ALTTEXT ouput range, isOutputRange, std,_range,primitives) with its template constraints.


Nitpick: ouput -> output

quickfur · 2018-01-03T22:07:36Z

std/format.d

+ * string toString();
+ * ---
+ *
+ * Where `W` allows a character accepting


What about "Where W is an output range that accepts ...".

That word order seems a little more clear. IMO.

Also, it really should be ref W. Accepting an output range by value is begging for trouble.

quickfur · 2018-01-03T22:09:51Z

std/format.d

+ * The template type does not have to be called `W`.
+ *
+ * The following overloads are also accepted for legacy reasons. It's recommended that
+ * any new code forgo these overloads for speed and attribute acceptance reasons.


What about also accepting a toString overload that takes an output range that can accept any range of characters? If efficiency is a concern here, it would make sense for toString to be able to take an output range that accepts a range of char/wchar/dchar, instead of needing a function call roundtrip per character.

Is it really just for legacy reasons though? What about classes? As soon as you templatize toString, it can't be virtual.

Hmm, that's true. Virtual toString methods could be problematic because format may not know how to construct an output range of a type that will be accepted by toString.

Perhaps one way out is use std.range.interfaces.OutputRangeObject. Since such an object is an output range, it can be recognized by format, and since it gives an opaque interface to arbitrary underlying output range implementations, it will also give us the flexibility to choose the best implementation used by format.

OTOH, since scope void delegate(const(char)[]) is already established in current code, and already allows arbitrary implementations, perhaps we should just retain support for it. Code that can afford to have toString templatized can still take advantage of a more flexible implementation of format.

@safety is still an issue for virtual toString functions, though. Well, as well as other attributes. I've no idea how to work around that.

quickfur · 2018-01-03T22:30:37Z

In general I like the direction this is going. The main concern I have is that we should support toString taking an output range that accepts ranges of characters, in order to minimize roundtrips when outputting strings.

JackStouffer · 2018-01-04T13:58:24Z

Changed the requirements to look for a string accepting output range instead of a char accepting one. Also added checks to make sure the output range is being taken by ref.

As for virtualizing toString, IMO the best option would be to use the string toString() overload in that case, as it's probably going to be faster with the inlining issue I mentioned. Plus, it can also be @safe in that case.

JackStouffer · 2018-01-04T15:00:43Z

#5995

JackStouffer · 2018-01-04T15:37:38Z

Thought about the round-trip per character issue some more, and I remembered that put solves this problem automatically for the user. Emphasized it in the docs.

quickfur · 2018-01-04T22:18:22Z

@JackStouffer It took me a few minutes to understand why and how exactly std.range.put solves the problem of round-trip per character. You might want to put in a slightly more detailed explanation than just pointing the reader to put. :-) Also, it would be nice if there's a hyperlink to the documentation for put somewhere.

quickfur

Looks good.

The only remaining question is what to do with class hierarchies that might wish to define a toString overload that takes a FormatSpec parameter, since the only ways that work with virtual functions will now be deprecated.

JackStouffer · 2018-01-05T14:13:41Z

@quickfur I'm not planning on deprecating the other overloads anytime soon, if ever. There's just too much code out there that uses it, and this is such a common function, that I'm not comfortable removing the delegate overloads from std.format.

quickfur · 2018-01-05T19:01:09Z

std/format.d

+ * which accepts characters. The template type does not have to be called `W`.
+ *
+ * The following overloads are also accepted for legacy reasons. It's recommended that
+ * any new code forgo these overloads for speed and attribute acceptance reasons.


You might want to reword this, if the older overloads are still recommended for class hierarchies where toString needs to be virtual.

JackStouffer · 2018-01-08T16:14:31Z

@quickfur Addressed comments

quickfur

Other than that, LGTM.

quickfur · 2018-01-08T17:44:35Z

std/format.d

+    }
+    // not using the overload enum to not break badly defined toString overloads
+    // e.g. defining the FormatSpec as ref and not const ref led this function
+    // to ignore that toString overload


Just a thought: to prevent similar breakages in the future, what about if the new unittest added above is changed to have non-trivial return values for the toString overloads, and the unittest actually checks for these values? The idea being that if a particular toString overload is ignored or not called correctly for whatever reason, the unittest would catch it.

wilzbach

We should enforce that the FormatSpec parameter is const ref. It will otherwise result in very confusing error messages as formatObject and formatValue already do so.

wilzbach · 2018-01-09T13:56:10Z

std/format.d

+ * string toString();
+ * ---
+ *
+ * Where `W` is an $(REF_ALTTEXT output range, isOutputRange, std,_range,primitives)


There shouldn't be any need for the underscore at _range here. No?

wilzbach · 2018-01-09T13:57:57Z

std/format.d

+ * Formatting a struct by defining a method `toString`, which takes an output
+ * range.
+ *
+ * It's recommended that any `toString` using $(REF_ALTTEXT output _rangew, isOutputRange, std,_range,primitives)


output ranges?

wilzbach · 2018-01-09T13:58:28Z

std/format.d

+ * range.
+ *
+ * It's recommended that any `toString` using $(REF_ALTTEXT output _rangew, isOutputRange, std,_range,primitives)
+ * use $(REF put, std,_range,primitives) rather than use the `put` method of the range


Again no need for _range

wilzbach · 2018-01-09T14:00:13Z

std/format.d

+
+        void toString(W)(ref W writer, const ref FormatSpec!char f) if (isOutputRange!(W, char))
+        {
+            // make sure to use std.range.primitives.put and not writer.put directly


Mentioning this in the summary page.

wilzbach · 2018-01-09T14:03:32Z

std/format.d

+ * this time using the `scope delegate` method.
 *
- *  `formatValue` also allows to reuse existing format specifiers:
+ * This method is now discouraged. Please use the output range method


$(RED ..) the first sentence. Or maybe just bold?

wilzbach · 2018-01-09T14:08:57Z

std/format.d

+        {T val = void; FormatSpec!Char f; static struct S {void put(Char s){}}
+        S s; val.toString(s, f);
+        // force toString to take output range by ref
+        static assert(ParameterStorageClassTuple!(T.toString!(S))[0] == ParameterStorageClass.ref_);}


How about enforcing that the second parameter (FormatSpec) is const and ref?

wilzbach · 2018-01-09T14:11:37Z

std/format.d

- * const void toString(scope void delegate(const(char)[]) sink);
- * const string toString();
+ * void toString(W)(ref W w, const ref FormatSpec fmt)
+ * void toString(W)(ref W w)


You don't have a public unittest showing this to the user. I know it's trivial, but I still would add one to avoid guaranteed questions on the NG.

wilzbach · 2018-01-09T19:50:41Z

std/format.d

+
+    formatValue(w, p, spec);
+    assert(w.data == "(16,11)");
+}


This public tests is failing, run with:

make -f posix.mak std/format.publictests

The line number on the error message is the one from the generated file.
(it probably makes sense to insert empty lines in it, s.t. the line numbers match up)

I'm not really sure how to debug this. This is green on the autotester

Compile Phobos

Run the extracted test with Phobos-dev:

unittest { import std.array : appender; import std.format; static struct Point { int x, y; void toString(W)(ref W writer, const ref FormatSpec!char f) if (isOutputRange!(W, char)) { // std.range.primitives.put put(writer, "("); formatValue(writer, x, f); put(writer, ","); formatValue(writer, y, f); put(writer, ")"); } } auto w = appender!string(); auto spec = singleSpec("%s"); auto p = Point(16, 11); formatValue(w, p, spec); assert(w.data == "(16,11)"); }

The issue is that w.data is Point(16, 11) here.

wilzbach · 2018-01-09T20:08:50Z

std/format.d

+    {
+        int x, y;
+
+        void toString(W)(ref W writer, const ref FormatSpec!char f) if (isOutputRange!(W, char))


If constraints should have the same indentation as their declaration:

void toString(W)(ref W writer, const ref FormatSpec!char f) if (isOutputRange!(W, char))

JackStouffer · 2018-01-12T14:41:18Z

@wilzbach Fixed. The public tests didn't have access to std.range.primitives.

JackStouffer · 2018-01-23T18:59:53Z

Ping @andralex

andralex

Great idea, thx @JackStouffer and sorry for the long wait. I thought it's a lot more complicated :)

schveiguy · 2018-01-23T23:08:00Z

changelog/toString.dd

+writeln(t); // writes "Custom toString"
+-------
+
+This has several benefits for the user. Firstly, this design is much friendlier


Firstly -> First, Secondly -> Second...

schveiguy · 2018-01-23T23:16:23Z

This is cool, I like it.

CyberShadow · 2018-11-16T10:48:38Z

This pull request seems to have exposed an -allinst DMD bug, thus causing a regression:
https://issues.dlang.org/show_bug.cgi?id=19406

JackStouffer requested review from andralex, quickfur and wilzbach January 3, 2018 19:57

JackStouffer added a commit that referenced this pull request Jan 3, 2018

Fix BigInt.toString overload

163b7db

See #5991 (comment) for more information

JackStouffer mentioned this pull request Jan 3, 2018

Fix BigInt.toString overload #5992

Merged

quickfur reviewed Jan 3, 2018

View reviewed changes

JackStouffer mentioned this pull request Jan 4, 2018

Changed all instances of FormatSpec in toString to const ref #5995

Merged

quickfur reviewed Jan 4, 2018

View reviewed changes

quickfur reviewed Jan 5, 2018

View reviewed changes

JackStouffer force-pushed the format-tostring branch from fb264e7 to 4445949 Compare January 8, 2018 16:14

quickfur reviewed Jan 8, 2018

View reviewed changes

wilzbach reviewed Jan 9, 2018

View reviewed changes

wilzbach added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Jan 9, 2018

JackStouffer force-pushed the format-tostring branch from 4445949 to 3d48156 Compare January 9, 2018 19:20

JackStouffer removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Jan 9, 2018

wilzbach reviewed Jan 9, 2018

View reviewed changes

JackStouffer force-pushed the format-tostring branch from cb0a421 to 356e0d6 Compare January 12, 2018 14:39

Modify std.format to recognize output range using toString

1dd6df3

JackStouffer force-pushed the format-tostring branch from 356e0d6 to 1dd6df3 Compare January 12, 2018 14:40

wilzbach added the @andralex label Jan 14, 2018

Use markdown in the changelog title

3b8f644

JackStouffer mentioned this pull request Jan 17, 2018

Fix Issue 15798 - std.algorithm.mutation.copy takes target by value #6039

Closed

andralex approved these changes Jan 23, 2018

View reviewed changes

andralex removed the @andralex label Jan 23, 2018

schveiguy reviewed Jan 23, 2018

View reviewed changes

JackStouffer changed the title ~~[RFC] Make Output Ranges The Preferred toString Method~~ [RFC] Make Output Ranges Using Methods The Preferred toString Option Jan 24, 2018

reworded the changelog entry

eb983d1

JackStouffer added Severity:Enhancement Merge:auto-merge labels Jan 24, 2018

dlang-bot merged commit 48b3d09 into dlang:master Jan 24, 2018

JackStouffer deleted the format-tostring branch January 24, 2018 05:50

JackStouffer mentioned this pull request Jan 24, 2018

Partially Fix Issue 10828 - Add output range version of toString functions to std.datetime.Date #6060

Merged

jondegenhardt mentioned this pull request Jan 25, 2018

Buffering output (possible enhancement) schveiguy/iopipe#17

Open

JackStouffer mentioned this pull request Jan 26, 2018

Work On Issue 10828 - Add output range versions of toString functions to std.datetime.TimeOfDay #6071

Merged

Uh oh!

[RFC] Make Output Ranges Using Methods The Preferred toString Option #5991

[RFC] Make Output Ranges Using Methods The Preferred toString Option #5991

Uh oh!

Conversation

JackStouffer commented Jan 3, 2018

Rationale

Uh oh!

dlang-bot commented Jan 3, 2018

Bugzilla references

Uh oh!

JackStouffer commented Jan 3, 2018

Uh oh!

JackStouffer commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur commented Jan 3, 2018

Uh oh!

JackStouffer commented Jan 4, 2018

Uh oh!

JackStouffer commented Jan 4, 2018

Uh oh!

JackStouffer commented Jan 4, 2018

Uh oh!

quickfur commented Jan 4, 2018

Uh oh!

quickfur left a comment

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jan 8, 2018

Uh oh!

quickfur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur Jan 3, 2018 •

edited

Loading

quickfur Jan 3, 2018 •

edited

Loading

wilzbach Jan 9, 2018 •

edited

Loading

wilzbach Jan 9, 2018 •

edited

Loading

wilzbach Jan 9, 2018 •

edited

Loading