Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added RefRange to std.range. #633

Merged
merged 1 commit into from Jun 21, 2012
Merged

Added RefRange to std.range. #633

merged 1 commit into from Jun 21, 2012

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Jun 15, 2012

Wrapper which effectively allows you to pass a range by reference. Both the
original range and the RefRange will always have the exact same elements.
Any operation done on one will affect the other. So, for instance, if it's
passed to a function which would implicitly copy the original range if it
were passed to it, the original range is not copied but is consumed as
if it were a reference type.

auto opAssign(RefRange rhs)
{
if(_range)
*_range = *rhs._range;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaviour would be very surprising to me. I would expect an assignment to always change the reference, and never the referee.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if you don't do that, then drop and popFrontN don't work. They do nothing. It becomes like if an array assignment assigned only the pointer and not the length.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, no. Bad analogy. If it assigned only the pointer, that would affect popBackN, not popFrontN. In any case, this line

    r = r[n .. r.length];

in popFrontN didn't work right if I didn't do this. I'll have to think through it again to figure out exactly why though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I remember now. The problem is that the assignment needs to affect the original range. If you just assign the pointer, then the original range is unaffected. So, what you then get with

r = r[n .. r.length];

is that the slice of the origanl range ( which opSlice returned wrapped in a RefRange) is assigned to the original range. So, the

r = r[n .. r.length];

acts like it was the slice of the original range which is assigned to the original range rather than having the wrapper then refer to a new range.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but I still don't like the fact that if I do the following:

SomeRange foo, bar;
auto a = refRange(&foo);
auto b = refRange(&bar);
RefRange!SomeRange x;

then the two lines

x = a;   // Only affects x
x = b;   // What, now b is assigned to a?

do two completely different things. I'm inclined to say that you should just drop support for slicing in RefRange, if this is the only reason for this behaviour. (Anyway, isn't it a bug if an algorithm does stuff like r = r[n .. r.length]? At least from the std.range documentation, there doesn't seem to be any requirement that opSlice return a range of the same type – just that it returns an input range.)

At any rate, there needs to be a null pointer check before rhs._range is dereferenced.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the whole point of the RefRange is to make it so that it is the original range, only now it's being passed around by reference, so it gets affected by the functions that it's passed to rather than being copied when it's passed. So, any operation occuring to the RefRange also occurs to the original. Saying

x = a;
x = b;

is tantamount to saying

a = b;

because x is a, just wrapped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I fixed the null dereference issue and added documentation to opAssign to clarify its behavior.

As for assigning a slice to the original, hasSlicing doesn't check for that, but it seems quite common that code assumes that a slice is assignable to the original. The same goes with save, though it actually tests the assignment. So, at present, it wouldn't work at all to use std.range or std.algorithm with a type that returned something different for a slice. And given the fact that it's quite common to assign a slice to the original, and that behavior is very much needed in general, I don't see how we could make it work otherwise. hasSlicing should probably be updated to reflect that requirement however.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, I have no more complaints. :-)

The hasSlicing debate is worth having, I think. It is not obvious (at least not to me) that a slice needs to have the same type as the slicee. At one point it was even suggested (by Walter, no less!) to make built-in arrays and slices different types... But that is for a different forum, and not this pull request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize that hasSlicing and save had to have the same type until I was working on this. It made me rethink how I was going to handle slices. And actually, they don't have to be the same type. They just have to be implicitly assignable to the original type. And in the case of arrays, a slice doesn't have the same type (though not due to that proposal). Rather, the slice is a tail-const version of the array. However, the tail-const version is assignable to the fully const version, so it works.

The problem is that you can't do that with user-defined types (at least not as far as I've been able to figure out). You can make opSlice and save return the exact same type, but then you can't operate on const user-defined ranges at all. Or you could make them return tail-const versions, but then the assignment fails, because the tail-const version is a completely different type (e.g. Range!(const int) instead of const Range!int). And I have been unable to figure out how to make Range!(const int) be implicitly assignable to const Range!int. I believe that the problem is that in implementing alias this or opAssign, you end up with a recursive template definition (certainly, the compiler gets OOM-ed for taking up too much memory when it tries to compile it). So, const and ranges continue not to get along very well. But that whole discussion really should be discussed in the newsgroup. I've been meaning to, after trying to solve the issues in relation to this, but I haven't gotten around to it yet. I'l have to remember to post on that soon.

@kyllingstad
Copy link
Member

Why aren't bidirectional and random-access range primitives included? (They can of course be added later without breaking anything, I just wonder if there is some technical reason preventing you from adding them.)

@jmdavis
Copy link
Member Author

jmdavis commented Jun 19, 2012

Bidirectional range and random-access range primitives are included.

@kyllingstad
Copy link
Member

@jmdavis:

Bidirectional range and random-access range primitives are included.

You are absolutely right – I have no idea how I missed that, when I was even actively looking for them.

{
return `import std.conv;` ~
`alias typeof((*_range)[begin .. end]) S;` ~
`static assert(hasSlicing!S, "S.stringof is not sliceable.");` ~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S.stringof should be outside the string literal – the inner one, that is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

It's a wrapper which effectively allows you to pass a range by
reference.
jmdavis added a commit that referenced this pull request Jun 21, 2012
Added RefRange to std.range.
@jmdavis jmdavis merged commit 51d4428 into dlang:master Jun 21, 2012
@jmdavis
Copy link
Member Author

jmdavis commented Jun 21, 2012

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants