std.container.rbtree: Return range elements by ref

[CyberShadow]

I don't see any downsides, and it will allow updating value types in-place as we're iterating over the tree.

Some counter-arguments I can think of:

• What if this is used to change the key such that the tree is no longer ordered?

  • Well you can do that already anyway if Elem is a reference type, it's not like we're returning const.

• What if the tree is changed while we're holding a reference to a node?

  • We're allocating nodes with new, so even if they're gone from the tree, it shouldn't result in a dangling pointer.

Did I miss anything?

CC @schveiguy

[dlang-bot]

Thanks for your pull request, @CyberShadow!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10755"

[CyberShadow]

Looks like the compiler was incorrectly allowing ref in foreach over such ranges, too:

import std.container.rbtree;

void main()
{
	auto t = new RedBlackTree!int();
	t.insert(2);
	foreach (ref r; t[]) // "ref" compiles, but doesn't do anything
		r++;
	assert(t[].front == 3); // fails
}

[Herringway]

Unfortunately, it seems the fix dlang/dmd#16381 is stalled or dead.

[CyberShadow]

Unfortunately, it seems the fix dlang/dmd#16381 is stalled or dead.

From what I see that seems to target the array index. Or does it also affect values of ranges with non-ref elements?

[Herringway]

Unfortunately, it seems the fix dlang/dmd#16381 is stalled or dead.

From what I see that seems to target the array index. Or does it also affect values of ranges with non-ref elements?

It does. It targetted two issues.

[schveiguy]

• What if this is used to change the key such that the tree is no longer ordered?

This is the only qualm I have about it. But you are right that returning by value may still allow mutation that would mess up the comparison.

I'd say that maybe a comment about this should be in there, but apparently the Range struct is private, so it doesn't even get ddoc'd!

[schveiguy]

You should add ref to the container's front and back as well, for consistency.

[CyberShadow]

I wrote this note:

* Note: The element is returned by `ref`, however,
* this should not be used to modify the element in a way which
* would affect the order defined by the `less` predicate.

but I'm not sure if it should be repeated 4 times.

[schveiguy]

Hm... maybe add it to the RedBlackTree ddoc comment?

[CyberShadow]

Done, how's this?

[schveiguy]

LGTM