Fix Issue 5945: redBlackTree printing

[JackStouffer]

Low hanging fruit:
https://issues.dlang.org/show_bug.cgi?id=5945

My solution is a little different than the original suggestion, as it isn't known if the redBlackTree factory function was used or not.

[schveiguy]

I think this is not the correct solution, as it allocates a temporary string that is immediately garbage.

The correct solution, I think, is to print the range, as identified in the bug report. As there is no way for RedBlackTree to hook that, I think the work to be done is not in RedBlackTree, but in format/writefln.

[yebblies]

Can't you just formattedWrite(sink, "%s", this[]) with the new toString?

[quickfur]

It's better to use this overload of toString:

void toString(scope void delegate(const(char)[]) sink);

[schveiguy]

Can't you just formattedWrite(sink, "%s", this[]) with the new toString?

I didn't know that could be done. Sounds like a good solution.

[JackStouffer]

@yebblies You mean like this?

override string toString() {
    import std.format : formattedWrite;
    import std.array : appender;

    auto str = appender!string();
    formattedWrite(str, "RedBlackTree(%s)", this[]);
    return str.data;
}

[schveiguy]

He means the toString variant that @quickfur alluded to. I'm not sure how it works on classes though.

[JackStouffer]

@schveiguy But yebblies' comment was made before quickfur's comment.

[schveiguy]

I know, both were referring to the new style of toString, which instead of creating a garbage strings, accepts a "sink" function to put all string output into.

[JackStouffer]

I don't understand that overload. Isn't toString supposed to return a string? What does the sink variable refer to? Are there any examples of that being used anywhere?

[schveiguy]

http://dlang.org/phobos/std_format.html#.formatValue

Scroll down until you see the overload that takes an aggregate with toString. It will explain how the overloads work.

Again, I'm not 100% sure how it works for classes, but I think it should work just by adding that overload (may have to provide a string returning overload too).

[JackStouffer]

Ok, so I changed the method to:

void toString(scope void delegate(const(char)[]) sink, FormatSpec!char fmt) const {
        sink("RedBlackTree(");
        sink.formatValue(this[], fmt);
        sink(")");
}

But now I am running into linker errors from std.format again, even though the unit tests pass. Original bug report: https://issues.dlang.org/show_bug.cgi?id=14959

But now the errors are

$ dmd/src/dmd test.d
Undefined symbols for architecture x86_64:
  "_D3std6format32__T14formatUnsignedTDFAxaZvTmTaZ14formatUnsignedFDFAxaZvmKxS3std6format18__T10FormatSpecTaZ10FormatSpeckbZv", referenced from:
      _D3std6format32__T14formatIntegralTDFAxaZvTlTaZ14formatIntegralFDFAxaZvxlKxS3std6format18__T10FormatSpecTaZ10FormatSpeckmZv in test.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
--- errorlevel 1

Where test.d is

import std.stdio, std.container;
void main() {
    auto t = redBlackTree(0, 7, 5, 2);
    writeln(t);
}

[DmitryOlshansky]

But now I am running into linker errors from std.format again, even though the unit tests pass. Original bug report: https://issues.dlang.org/show_bug.cgi?id=14959

Just add the unittest and we'll see what the auto-tester says

[JackStouffer]

Well the problem is that the tests don't cover the toString method, so even if the tests pass it doesn't really say anything. Also, I now don't have a way to check if the toString method actually does what I want it to do.

[DmitryOlshansky]

Also, I now don't have a way to check if the toString method actually does what I want it to do.

assert(somRbT.to!string == "[....]'); 

No?

[JackStouffer]

At the time I was referring to the linker errors and my inability to compile the program. But after I re-downloaded dmd and druntime I was finally able to compile the test program in the issue.

Thank you for reminding me about this. I have added your unit test now.

[DmitryOlshansky]

Otherwise LGTM

[JackStouffer]

@DmitryOlshansky Added extra tests.

[DmitryOlshansky]

Hm... Move that to toString method itself? Since RB-Tree is a template that should cut down on number of imports if RB-Tree is not instantiated but std.container is imported.

[JackStouffer]

Unfortunately the void toString(scope void delegate(const(char)[]) sink, FormatSpec!char fmt) const overload doesn't work if std.format is a local import.

[JakobOvrum]

Could do something like:

template toString(DG)
{
    import std.format;
    void toString(scope DG sink, FormatSpec!char fmt) const
    {
        ...
    }
}

override string toString() const
{
    auto app = appender!string();
    toString(app, FormatSpec!char("%s"));
    return app.data;
}

edit:
Err, scratch the runtime polymorphic overload, clearly it would still require FormatSpec.

[DmitryOlshansky]

I guess we have to make do with global import for now

[DmitryOlshansky]

Leaving last word to @schveiguy

[schveiguy]

Well, it's better than the current behavior, and adjusting the format later isn't going to make a huge difference.

[schveiguy]

Shouldn't this be documented? I'm honestly not sure, because I don't know what the policy is for documenting toString functions.

[JackStouffer]

Doing a search through Phobos I see examples of both documentation and no documentation, so your call.

[schveiguy]

I say err on the side of documentation for public functions :)

[JackStouffer]

Talk about damning with faint praise ;)

[JackStouffer]

Added documentation.

[schveiguy]

This returns void, so starting the doc with "Returns" is not correct.

I think this might be acceptable:

Formats the RedBlackTree into a sink function. For more info see $(D std.format.formatValue)

[schveiguy]

Talk about damning with faint praise ;)

Heh, I will rephrase: I don't like the redundant brackets, but the output is not confusing or misleading, it just looks ugly to me :) Bikeshedding shouldn't derail this.

In any case, I appreciate the effort to fix the bug, and I think it's an acceptable solution.

[JackStouffer]

Fixed documentation

[schveiguy]

Auto-merge toggled on

[JackStouffer]

Woohoo! My first real contribution to Phobos!

[DmitryOlshansky]

Woohoo! My first real contribution to Phobos!

Congrats! ;)