add documentation example for RBTree

[wilzbach]

In reference to #4039 and #4040 also an example for Red Black Trees.

[schveiguy]

ping @CyberShadow Something weird happened here with doc tester. One file changed, 12k changes in docs?

[wilzbach]

Difference is due to the different version ...

-        <p><span class="smallprint">version 2.070.1 <span class="separator"><br></span>
+        <p><span class="smallprint">version 2.070.2 <span class="separator"><br></span>

[schveiguy]

I think idiomatic D would be to use the factory method, which uses IFTI to infer the type.

auto rbt = redBlackTree(3, 1, 4, 2, 5);

[CyberShadow]

Difference is due to the different version ...

I'm not sure why that happened, but it seems to have reconciled itself now.

[schveiguy]

One other thing to demonstrate may be how inserting a duplicate on a normal tree doesn't change the rbtree, and returns 0 to indicate it couldn't be added.

[wilzbach]

One other thing to demonstrate may be how inserting a duplicate on a normal tree doesn't change the rbtree, and returns 0 to indicate it couldn't be added.

Nice idea - done!

Note, the fact that you are using iota means you must use the class ctor. But you could technically do this with the convenience function (I'm surprised it doesn't work with iota):

I added wrappers for the convenience function for InputRange. It was a bit tricky because there is string which can be both char[] and an InputRange. That's why I now explicitly exclude string as there might be the case redBlackTree("foobar"). If there is a better way to check for this single special case, let me know.

[schveiguy]

Nice additions! I'd like to see unit tests specifically for all of these though.

[JakobOvrum]

When I initially fixed the constructor to support range construction I intentionally did not add these because the redBlackTree overload set is terribly designed. With this, there are 8 overloads of redBlackTree just to accomodate arbitrary argument order, one of which is a classical case of boolean blindness. Blergh.

[wilzbach]

Do you have a better idea? Imho ranges are the most common type in D, so redBlackTree should support it.
Idea: You could have a redBlackTreeDups that always adds the parameter allowDuplicates, but I don't know whether it's worth it ;-)

It would be nice to have named parameters in D at some point though ...

[schveiguy]

I think the string check is probably fine. Strings are definitely special in D, so even though they are arrays and arrays should match the variadic version, nobody I think would expect redBlackTree("hi") to produce a RedBlackTree!dchar. I don't think any other array types should be treated that way.

[wilzbach]

Nice additions! I'd like to see unit tests specifically for all of these though.

done.

I think the string check is probably fine. Strings are definitely special in D, so even though they are arrays and arrays should match the variadic version, nobody I think would expect redBlackTree("hi") to produce a RedBlackTree!dchar. I don't think any other array types should be treated that way.

Excellent :)

[schveiguy]

LGTM

[JakobOvrum]

Just saw the string stuff, please no! This makes the function utterly useless in generic code. Every specialization of such caliber requires that the caller also specializes to achieve generic behaviour. It doesn't scale and is likely to surprise programmers.

redBlackTree("literal here") is a contrived case and can easily be changed to redBlackTree(["literal here"]) or redBlackTree(only("literal here")).

[wilzbach]

Just saw the string stuff, please no! This makes the function utterly useless in generic code. Every specialization of such caliber requires that the caller also specializes to achieve generic behaviour.

Hmm that makes sense, so you suggest just to change the otherwise failing unittest?

I was just worried that there might be code that would rely on this behavior ...

[JakobOvrum]

Yes, redBlackTree("hello") should create a red-black tree of individual characters. Range interfaces are general and arrays present a range interface. We use a lot of specialization for array-specific optimizations in Phobos but not for semantic deviation.

An (admittedly still contrived) example for good measure. The user guesses and types redBlackTree("hello") and gets what they wanted, which happened to be a red-black tree of one string element. Later the user changes it to redBlackTree("hello".asUpperCase) and suddenly their code stops compiling, or worse, compiles but exhibits completely different behaviour upon executing, because semantics have changed.

Treating all ranges equally eliminates such clumsy issues.

[wilzbach]

Treating all ranges equally eliminates such clumsy issues.

Done with fc67d6f. Converting an object to a string for tests has been criticized on other PRs of mine, but it was already there, so I kept the "style".

[schveiguy]

@JakobOvrum, your arguments are well founded. I don't see anyone expecting redBlackTree("someLiteral") to result in a red black tree of dchar (actually, I think it will result in RedBlackTree of char?), but this is easily worked around.

I'm actually thinking, however, that redBlackTree that accepts a range should constrain out arrays of any type, since the variadic versions will accept them already.

[wilzbach]

I'm actually thinking, however, that redBlackTree that accepts a range should constrain out arrays of any type, since the variadic versions will accept them already.

If I would add those checks, it would result to the old behavior if you only have one argument as strings are arrays.

auto rt2 = redBlackTree!string("hello"); //   RedBlackTree(["hello"])

So I think the constraints should be more on the other side, but as the compiler already prefers the specific range function over the variadic function, there is no need to add an extra constraint.

[schveiguy]

I just tested, and I think it would result in RedBlackTree(['h', 'e', 'l', 'l', 'o']). Are you sure?

void foo(T)(T[]...)
{
    pragma(msg, T);
}

void main()
{
    foo("hello"); // outputs immutable(char)
}

[wilzbach]

I just tested, and I think it would result in RedBlackTree(['h', 'e', 'l', 'l', 'o']). Are you sure?

1. It should result in 'e', 'h, 'l', 'o' (by default no duplicates).

2. You even have a test for this in master:

https://github.com/D-Programming-Language/phobos/blob/master/std/container/rbtree.d#L1864

Compare with the new test with range input (how it should be)

https://github.com/greenify/phobos/blob/docu-rbtree/std/container/rbtree.d#L1972

3. Yep I tested by adding isArray constraints.

[schveiguy]

You even have a test for this in master:

That test uses explicit template instantiation.

[wilzbach]

I fully agree with @schveiguy that we need to have a better way - I recently started yet another discussion on this
https://forum.dlang.org/thread/ngzayoxydjglsbwdfmqj@forum.dlang.org

The main point here is that the user shouldn't need to specify an argument he doesn't care about.

[JakobOvrum]

I think the point is, with the current overload set, you can specify whatever you want (element type, comparison function, and whether it should take duplicates) without having to re-specify the defaults. I like how Phobos does this.
I agree that the docs could be written better, but this is a frequent problem in Phobos, especially with IFTI templates, I think we need a more general solution.

I don't know which functions you're referring to, but if it's something like find - those overloads do slightly different things under the same name, while redBlackTree has overloads simply to allow for arbitrary argument order for the same thing. find does have the benefit of having fewer parameters so it doesn't encounter this problem.

RBT requires a strict ordering for elements. We agreed I think that in some cases (e.g. floating point), having weird results are OK because it doesn't appear much in practice. I think there may even be some code that checks for this (I vaguely remember).

Not all types support the comparison operators.

[schveiguy]

I don't know which functions you're referring to

What I mean is, we have overloads for many different reasons, many of them have to do with separating implementation based on templates.

In this case, it's overloading based on template parameter types and quantities -- what overloading is for. I can agree that the documentation is verbose, and could be simplified, but I like that the calls are simple and straightforward.

Not all types support the comparison operators.

Most do. Even custom structs do, as long as you have members that do. I think it's the most useful default. What would you suggest otherwise?

In dcollections, the comparison is done via TypeInfo.compare, but I think this is cleaner.

[wilzbach]

I can agree that the documentation is verbose, and could be simplified, but I like that the calls are simple and straightforward.

I absolutely share your opinion @schveiguy

Is it possible to hide all the different methods, so that the generated documentation is shorter?

Here is my short summary/opinion:

1. I am not yet an expert with D's templates, but afaict templates don't solve the problem of having convenience overloads?
2. Whether we support the convenience overloads or require the user to specify non-needed parameters for shorter documentation is a design question that should be best discussed in all details on the forum
3. In any case of the outcome of 2): We can't change the API for rbTree as easy as this, so I think it's more than fair to expand this with ranges and see in which direction 2) goes?

Input of other DLang members would be kind, so that we can move forward with this PR :)

[quickfur]

Here's one way (not necessarily the best, but it's a way) of simplifying the docs when you have a large number of overloads for different template argument types that are mostly implementation-specific (i.e. the user shouldn't need to know about them):

/// Function docs go here
auto myFunc(A...)(A args)
    if ( ... /* contraints on arguments of *all* overloads */ )
{
    return myFuncImpl!A(args);
}

// Implementation-specific overloads go here:
auto myFuncImpl(A...)(A args) if (A.length == 2) { ... }
auto myFuncImpl(A...)(A args) if (is(A[0] == string)) { ... }
... etc.

Basically, the user sees (both in code and in the generated docs) a single, unified API with a single set of constraints that apply to all possible template arguments; while the implementation may be split up into multiple cases with complex sig constraints based on which case(s) of template arguments they are specialized for.

[wilzbach]

Basically, the user sees (both in code and in the generated docs) a single, unified API with a single set of constraints that apply to all possible template arguments;

That's a really nice idea, but afaict it doesn't work well with the current overloads and optional template parameters, especially because sth. like this redBlackTree!("a > b", true, string)("hello", "world") is supported as well.
Is there a way to make it work nicely with optional template parameters?

[schveiguy]

I think we should leave the overloads as is, especially in this PR.

I'm not a fan of docs that simplify to that level. What I think might look good is to reorganize the docs so the range and variadic versions are documented together, call the third argument "stuff" in both cases. That will at least cut down the docs in half.

[wilzbach]

I think we should leave the overloads as is, especially in this PR.

+1

I'm not a fan of docs that simplify to that level. What I think might look good is to reorganize the docs so the range and variadic versions are documented together, call the third argument "stuff" in both cases.

done.

That will at least cut down the docs in half.

How do you combine them?

auto redBlackTree(bool allowDuplicates, Stuff)(Stuff range)
auto redBlackTree(bool allowDuplicates, E)(E[] elems...)

[schveiguy]

How do you combine them?

Document both above the first function, then use /// ditto on the second function. DDoc will list both functions under the same docs.

[schveiguy]

BTW, looking at it now, it's probably not good to call both arguments "stuff". Sorry for that suggestion. Keeping the types separately named will help with the docs.

[schveiguy]

Meh, looking at the docs, they are already all Ditto'd.

And very little documentation to explain what each parameter does. You can probably just add a bunch of Params documentation, and this will make the whole thing much more understandable.

[wilzbach]

And very little documentation to explain what each parameter does. You can probably just add a bunch of Params documentation, and this will make the whole thing much more understandable.

Done. Also added range examples to the "ddoced" unittest. I think it makes sense to add asserts there too - what do you think @schveiguy?

BTW, looking at it now, it's probably not good to call both arguments "stuff". Sorry for that suggestion. Keeping the types separately named will help with the docs.

No worries, but can let's try to move this PR forward - it was really planned as a trivial edit ;-)

[schveiguy]

Great, just need one more for range.

[wilzbach]

wouldn't that be confusing to the user? Imho it's better to state that it can be used for both (see brackets).

[schveiguy]

Well, the parameter is called range though.

Maybe name the documented parameter elems/range? Or rename actual function parameter elems?

[wilzbach]

I like elems/range the most - updated ;-)

[schveiguy]

Bleh, ddoc seems not to like elems/range. :( It's missing in the docs now. WTF

[schveiguy]

Try the other way (2 separate parameter docs), so we can put this to bed.

[dlang-bot]

Fix	Bugzilla	Description
✓	15596	strip with delimiter?

[wilzbach]

Try the other way (2 separate parameter docs), so we can put this to bed.

done & squashed all commits.
Sorry about the spam with dlang bot - did a mistake on the first rebase.

[schveiguy]

Nice, I think it LGTM. @quickfur @JakobOvrum any issues with this?

[wilzbach]

PR reviewer ping pong :)

[schveiguy]

Auto-merge toggled on