Fix issue 15941 -- rbtree no longer supports classes #4353
Conversation
|
|
ping @MartinNowak, can we get this in 2.071.1? |
| sink(")"); | ||
| static if(is(typeof((){FormatSpec!(char) fmt; formatValue((const(char)[]) {}, ConstRange.init, fmt);}))) | ||
| { | ||
| void toString(scope void delegate(const(char)[]) sink, FormatSpec!char fmt) const { |
There was a problem hiding this comment.
the problem with that delegate signature is that toString can not be @safe because you can't call put(const(char)[]) @System delegate.
One solution would be to make it trusted, by that is bad as well. I think at some point we have to make a choice what is the lesser evil. Disallow @safe toString by default or be @trusted and hope.
There was a problem hiding this comment.
It's part of a template so all that is inferred
There was a problem hiding this comment.
Oh you mean the delegate? Yeah, one possibility would be to template that parameter
There was a problem hiding this comment.
toString of Object doesn't have this form I don't think. But in any case we can save this for another time, the important thing is that this is fixing a regression where you can't use classes as element types
|
Interesting difference between ddox and ddoc here. I left the docs outside static if, and it seems to be fine for ddoc, but ddox has removed the toString documentation. ping @s-ludwig |
|
I changed the docs so hopefully they work with ddox. |
Nope, still didn't help. Not sure what to do... |
|
This is an issue related to DMD's JSON outuput. Related issue: dlang/ddox#86 I think at some point we'll switch to the libdparse backend for generating the documentation, though, in which case this will be fixed. |
|
@s-ludwig OK, thanks for letting me know. I will undo that |
|
Fixed up the docs. |
|
I find it a little funny that my first contribution to Phobos lead to a serious regression. LGTM |
| element type can be formatted. Otherwise, the default toString from | ||
| Object is used. | ||
| */ | ||
| static if(is(typeof((){FormatSpec!(char) fmt; formatValue((const(char)[]) {}, ConstRange.init, fmt);}))) |
There was a problem hiding this comment.
This was a PITA to do. At first, I did if(is(typeof(formatValue((const(char)[]){}, ConstRange.init, FormatSpec!(char).init)), but formatValue takes the format spec by ref.
Perhaps there is a nicer trait. I'm not sure.
|
@JackStouffer not your fault: 1) I find it funnier that we had no test of classes as an element type, 2) the |
Simple -- if type doesn't support formatting the range, then don't define
toString. This means it defaults toObject.toStringin those cases, but this isn't really a problem.There are likely better ways to do this, but this fixes the regression now.
Need to see how docs get generated to find out if I did that part right.