Fix Issue 13971

[Kozzi11]

https://issues.dlang.org/show_bug.cgi?id=13971

[JakobOvrum]

Although nullary toString like this is important to include for classes so it overrides Object.toString, please add sink-based toString.

[Kozzi11]

Sorry but I dont't know what you mean by this (I even don't know what sink-based means). Can you please give me some example?

[JakobOvrum]

void toString(scope void delegate(const(char)[]) sink, FormatSpec!char fmt)
{
    import std.format : formatValue;
    import std.internal.scopebuffer : scopeBuffer;
    char[256] buffer; // arbitrary size
    auto sBuffer = scopeBuffer(buffer[]);
    formatValue(sBuffer, symbol, fmt);
    sink(sBuffer[]);
}

It might be prudent to wrap the usage of ScopeBuffer in @trusted.

A better implementation might be to iterate this[] and calling the sink with the stringified result of each element, although this would need to take custom toString (on the range type) and such into account, so this logic probably belongs somewhere in std.conv or std.format, not anywhere in the std.container package.

See also the wiki page for sink-based toString.

(Note also the brace style; this style is the Phobos convention)

[JakobOvrum]

Otherwise LGTM assuming the issue is valid... I'm wary about any change that conflates containers and ranges, as it cannot be taken to its ultimate conclusion. I'm inclined to think that stringification of the container should print its identity (ala SList!int@0xDEADBEEF) - it's really easy for the user to do myContainer[] when printing the elements is desired. The user has to learn about the opSlice operator anyway for the vast majority of container use cases.

[Panke]

TL;DR
I am opposed to this change, because printing the output as well is easy with the current API but printing only type and identity becomes impossible with this PR and current API.

First I agree with jakob about conflating containers and ranges. This will just move the problem (the misunderstanding of how containers and ranges are different) to the next place where a container behaves different from a range. The correct thing to do is to require the user to use opSlice if he needs a range over the container.

Second the question is, what should toString be used for and what is the most convenient thing for the developer. If you print something pretty for your users, you'll usually have to write your own function anyway since phobos cannot anticipate all desired formats. This makes toString more of a tool for the developer, to print the contents inside a debugger or during println-debugging. Having identity and type information is useful here, however

Array!int(RefCounted!(Payload, cast(RefCountedAutoInitialize)0 (RefCountedStore(20D9590)))

Is much to verbose and contains useless information. I think

Array!int@20D9590

would be a good compromise, if 20D9590 identifies the payload uniquely, like Jakob suggested. Printing the contents alongside with this is often not what I want, because the container might just be to big for an output of all it's contents to be meaningful.

And if I want to print the output as well, it's as hard as:

writefln("%s%s", myArray, myArray[])

I am opposed to this change, because printing the output as well is easy with the current API but printing only type and identity becomes impossible with this PR and current API.

[Kozzi11]

@Panke @JakobOvrum

You convinced me that this is not a good solution, maybe we can just improve doc with some example so it would much more clear how to print containers.

[schveiguy]

So close this?

[schveiguy]

Huh, I don't know why I didn't read this fully before. I have some comments:

because printing the output as well is easy with the current API but printing only type and identity becomes impossible with this PR and current API.

This can be fixed, the code is in std.format, we can expose it, this is not a problem that should hinder this change.

In general, I think writeln(somearray) should print the array contents. It makes no sense that it would print the type. AA's and builtin arrays do not work this way, so I think we should fix this. In general, I think writeln(typeid(x)) should print the type, but in this case, the format function goes further, writeln(typeid(arr)) prints for the above example std.container.array.Array!int.Array (maybe that could also be fixed).

I agree that the toString function should use the sink version. If you don't want to do this work still, understood, I may open my own PR and do this.

[JakobOvrum]

It makes no sense that it would print the type.

I think it should print the container's identity.

AA's and builtin arrays do not work this way, so I think we should fix this.

Neither AA's nor builtin slices abide by the Phobos container concept, they're not even close. I think builtin slices in particular are not a good argument as they are ranges.

I stand by my argument from before. We need to improve documentation so the user understands the container/range separation; ad-hoc conflation of the two concepts in Phobos does not solve the underlying problem, it just makes it worse.

Containers have historically had really poor documentation, and indeed the concept is not completely fleshed out yet. It's too late to change inbuilt types (T[new], anyone?), but we can do containers right. We have to weigh consistency with the design of inbuilt but arguably suboptimal types against good design. I also think it is a possibility that idiomatic D in the future will strongly favour library containers and ranges over in-built types considering the inherent performance difficulties of AAs and dynamic arrays, so I would really like to see strong arguments that don't depend on the behaviour of inbuilt types.

[schveiguy]

We need to improve documentation so the user understands the container/range separation; ad-hoc conflation of the two concepts in Phobos does not solve the underlying problem, it just makes it worse.

Documentation doesn't fix this. 9 times out of 10, a user will want to print a container and find the current output useless. It doesn't make a whole lot of sense to say RTFM at that point.

I think printing a container should do the most expected thing, and leave the obtuse thing for a specialized function.

Containers have historically had really poor documentation, and indeed the concept is not completely fleshed out yet.

Yes, I agree, we should document that printing a container prints its elements.