Added writer overload of toString to Appender #6235

JackStouffer · 2018-02-28T15:31:29Z

No description provided.

dlang-bot · 2018-02-28T15:31:32Z

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.

n8sh · 2018-02-28T18:58:48Z

std/array.d

+        // different reserve lengths because each element in a
+        // non-string-like array uses two extra characters for `, `
+        static if (isSomeString!A)
+            app.reserve(_data.arr.length + 10);


Why not just _data.arr.length if the source is a string?

EDIT: Ah nevermind I see, you also add typeof(this).stringof and parentheses around the result. But 10 chars means the array will always be too short, since it is just the length of "Appender()" and doesn't include "!something".

JackStouffer · 2018-03-01T00:26:20Z

There's some sort of odd call to appender's toString happening somewhere in conv.to. Currently investigating.

JackStouffer · 2018-03-02T17:11:50Z

I found a bug in hasToString that I need help fixing. @n8sh @wilzbach

Consider

import std.format;
import std.range;

struct A(T)
{
    string toString() const { return ""; }
    void toString(W)(ref W w) const { put(w, ""); }
}

struct B(T)
{
    string toString()() const { return ""; }
    void toString(W)(ref W w) const { put(w, ""); }
}

void main()
{
    pragma(msg, hasToString!(A!(int), char)); // 1
    pragma(msg, hasToString!(B!(int), char)); // 5
}

It has to do with this line

phobos/std/format.d

Line 3627 in 8f00945

    
           static assert(ParameterStorageClassTuple!(T.toString!(S))[0] == ParameterStorageClass.ref_);}

Because the first toString overload in A is not a template but a function, the attempted instantiation of the toString template inside hasToString always fails despite the fact that there is an existing toString template.

JackStouffer · 2018-03-02T17:12:49Z

This needs to be fixed before we release 2.079

JackStouffer · 2018-03-05T18:43:25Z

Needs #6247

JackStouffer · 2018-03-12T19:20:34Z

Ready to go.

n8sh · 2018-03-12T19:23:13Z

std/array.d

+        // Even though the Appender might represent a string,
+        // make a copy of the data anyway because we don't want
+        // manipulations of the toString result to change the
+        // data of the Appender


If the element type is immutable how could manipulations of the toString result change the data of the Appender?

hmmm, you're right, I was thinking more of the char[] case. In any case, the code remains the same regardless to keep it dry and to use the speedup of Appender.

I'll just remove the comment.

wilzbach

LGTM + a few nits

wilzbach · 2018-03-12T19:32:55Z

std/array.d

+        auto spec = singleSpec("%s");
+        // different reserve lengths because each element in a
+        // non-string-like array uses two extra characters for `, `
+        static if (isSomeString!A)


FWIW we wanted to deprecate this trait...

#5284, #5137

isSomeString is far too ubiquitous to deprecate IMO. Johnathan already modified it to not recognize enums, which I think was the main issue.

wilzbach · 2018-03-12T19:37:59Z

std/array.d

+        // Even though the Appender might represent a string,
+        // make a copy of the data anyway because we don't want
+        // manipulations of the toString result to change the
+        // data of the Appender


We could also support const(string) toString()() const ...

wilzbach · 2018-03-12T19:45:59Z

std/array.d

+        // different reserve lengths because each element in a
+        // non-string-like array uses two extra characters for `, `
+        static if (isSomeString!A)
+            app.reserve(_data.arr.length + 15);


Appender!(int[])() has 18 character, so maybe + 20 or +25 or maybe even better with Unqual!(typeof(this)).stringof.length + 2?

wilzbach · 2018-03-12T19:47:27Z

std/array.d

+        static if (isSomeString!A)
+            app.reserve(_data.arr.length + 15);
+        else
+            app.reserve((_data.arr.length * 3) + 15);


Hmm, I assume this is the conservative estimate that of X, being three characters which does make sense, though I think it would be nice to add a comment for future readers about this conservative estimate.

wilzbach · 2018-03-12T19:50:50Z

std/array.d

+     */
+    string toString() const
+    {
+        auto app = appender!string();


In theory we could also do const(string) toString()() const { return app.data } for appender!string, but that's a potential optimization for the future.

You still need to add Unqual!(typeof(this)).stringof

Fair point. Though that could be solved by allowing toString to return a range, but that's a story for a different PR.

JackStouffer · 2018-03-12T20:08:41Z

That weird std.conv error is showing up again. I'll fix this when I get a chance.

JackStouffer · 2018-03-13T14:18:13Z

std/uni.d

@@ -7102,7 +7102,7 @@ public: // Public API continues
    Returns:
        length of grapheme cluster
 +/
-size_t graphemeStride(C)(const scope C[] input, size_t index)
+size_t graphemeStride(C)(const scope C[] input, size_t index) @safe pure


After going all the way down the rabbit hole, I found out that due to some compiler bug, DMD marked this as @system when it was instantiated from std.formatRange from std.conv.textImpl. No idea why. Manually adding the attributes fixes the whole issue.

JackStouffer added the Severity:Enhancement label Feb 28, 2018

JackStouffer requested review from wilzbach and PetarKirov as code owners February 28, 2018 15:31

JackStouffer force-pushed the appender-tostring branch from 797f8a2 to c6f71cb Compare February 28, 2018 15:34

n8sh reviewed Feb 28, 2018

View reviewed changes

JackStouffer added the Review:Needs Work label Mar 1, 2018

JackStouffer added Merge:Blocked and removed Review:Needs Work labels Mar 9, 2018

JackStouffer force-pushed the appender-tostring branch from c6f71cb to d498946 Compare March 12, 2018 19:05

JackStouffer requested a review from andralex as a code owner March 12, 2018 19:05

JackStouffer force-pushed the appender-tostring branch 2 times, most recently from 55a5d81 to becc9a0 Compare March 12, 2018 19:08

JackStouffer removed the Merge:Blocked label Mar 12, 2018

JackStouffer force-pushed the appender-tostring branch from becc9a0 to 1f7f16f Compare March 12, 2018 19:12

n8sh reviewed Mar 12, 2018

View reviewed changes

JackStouffer force-pushed the appender-tostring branch from 1f7f16f to ea5488a Compare March 12, 2018 19:40

wilzbach approved these changes Mar 12, 2018

View reviewed changes

wilzbach reviewed Mar 12, 2018

View reviewed changes

JackStouffer force-pushed the appender-tostring branch 2 times, most recently from b61881f to a8f5142 Compare March 12, 2018 19:55

JackStouffer added the Merge:auto-merge label Mar 12, 2018

JackStouffer force-pushed the appender-tostring branch from a8f5142 to 60743c8 Compare March 12, 2018 20:00

dlang-bot removed the Merge:auto-merge label Mar 12, 2018

JackStouffer added the Merge:auto-merge label Mar 12, 2018

JackStouffer removed the Merge:auto-merge label Mar 12, 2018

JackStouffer force-pushed the appender-tostring branch 2 times, most recently from 32a28a3 to ada7d5f Compare March 13, 2018 12:36

Added writer overload of toString to Appender

7beea03

JackStouffer force-pushed the appender-tostring branch from ada7d5f to 7beea03 Compare March 13, 2018 14:16

JackStouffer requested a review from DmitryOlshansky as a code owner March 13, 2018 14:16

JackStouffer commented Mar 13, 2018

View reviewed changes

JackStouffer added the Merge:auto-merge label Mar 13, 2018

dlang-bot merged commit 36edc8b into dlang:master Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added writer overload of toString to Appender #6235

Added writer overload of toString to Appender #6235

JackStouffer commented Feb 28, 2018

dlang-bot commented Feb 28, 2018 •

edited

Loading

n8sh Feb 28, 2018 •

edited

Loading

JackStouffer commented Mar 1, 2018

JackStouffer commented Mar 2, 2018

JackStouffer commented Mar 2, 2018

JackStouffer commented Mar 5, 2018

JackStouffer commented Mar 12, 2018

n8sh Mar 12, 2018

JackStouffer Mar 12, 2018

wilzbach left a comment

wilzbach Mar 12, 2018

JackStouffer Mar 12, 2018

wilzbach Mar 12, 2018

wilzbach Mar 12, 2018

wilzbach Mar 12, 2018

wilzbach Mar 12, 2018

JackStouffer Mar 12, 2018

wilzbach Mar 12, 2018

JackStouffer commented Mar 12, 2018

JackStouffer Mar 13, 2018

Added writer overload of toString to Appender #6235

Added writer overload of toString to Appender #6235

Conversation

JackStouffer commented Feb 28, 2018

dlang-bot commented Feb 28, 2018 • edited Loading

Bugzilla references

n8sh Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

JackStouffer commented Mar 1, 2018

JackStouffer commented Mar 2, 2018

JackStouffer commented Mar 2, 2018

JackStouffer commented Mar 5, 2018

JackStouffer commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackStouffer commented Mar 12, 2018

Choose a reason for hiding this comment

dlang-bot commented Feb 28, 2018 •

edited

Loading

n8sh Feb 28, 2018 •

edited

Loading