www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Proposal to deprecate "retro.source"

reply "monarch_dodra" <monarchdodra gmail.com> writes:
I just got snagged in a "funny" bug.

Using a generic template, I wanted a which shrank from the back. 
To do this, I made a call to "find(r.retro, stuff).source"

The idea being: Search from the back, and then extract the 
forward-iteration range from the result of find.

I got greeted with an "unknown property: source": WTF?

Turns out that in my function, "r" was already a retro range. In 
this condition, retro(r) un-retroed my range, which was all fine 
and well, but now it didn't have "source" anymore :/

The solution is actually quite easy, and already implemented: 
Just retro again!

The problem though is in the way the documentation "The original 
range can be accessed by using the source property" and "Applying 
retro twice to the same range yields the original range": Looks 
like we forgot to notice these two sentences are contradicting.

--------
To this end, I propose to deprecate "retro.source", and simply 
reword as:
*"Applying retro twice to the same range yields the original 
range"
*"The original range can be accessed by applying retro"

--------
I'd also like to not that this would avoid some *incredibly* 
unsafe code, that checks if R is of type retro by simply checking 
the *source* field... eg: to check if R1 is the retro of R2:
"is (typeof(r1.source) == typeof(r2))" //BAD! Could also be take, 
or who knows what!
which could be replace by
"is (typeof(r1.retro()) == typeof(r2))" //Good: Safe.

Thoughts?

(or if not deprecate, at least reword the documentation...)
Nov 06 2012
next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, November 06, 2012 18:05:27 monarch_dodra wrote:

 Thoughts?


We need to figure out how we want to handle source in general. There are some 
cases where it's really useful to be able to get at the original range - the 
prime example of this being functions like remove in std.container. The 
problem is that in most (all?) cases, you can't really use source without 
understanding how the wrapper range works, because the source doesn't 
necessarily have the same number or order of elements as the wrapper (e.g. if 
filter had source, the elements that source had could be very different from 
what the filtered range had, or in the case of retro, the order is completely 
different). In some cases, like retro and take, it's quite easy for the 
function accessing source to know how to handle it properly. In others, it 
gets far more complicated.

Andrei has expressed a desire to expose source in general. I don't know how 
wise that is or isn't, but it's something that needs to be explored.

In the case of retro, I think that it would good to have source exposed for 
std.container's use. It's easy for std.container to understand what retro's 
supposed to do, and use it accordingly, and I think that it would be silly for 
it have to call retro on the retroed range to do that. I do agree however that 
in general, it doesn't make sense to access source.

As for


 The problem though is in the way the documentation "The original 
 range can be accessed by using the source property" and "Applying 
 retro twice to the same range yields the original range": Looks 
 like we forgot to notice these two sentences are contradicting.


I don't see anything contradictory at all. If you call retro on a retroed 
range, you get the original. If you access source, you get the original. 
Where's the contradiction? In both cases, you get the original range.

So, I see no reason to deprecate retro's source.

- Jonathan M Davis
Nov 08 2012
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 8 November 2012 at 09:18:54 UTC, Jonathan M Davis 
wrote:

 In the case of retro, I think that it would good to have source 
 exposed for
 std.container's use. It's easy for std.container to understand 
 what retro's
 supposed to do, and use it accordingly, and I think that it 
 would be silly for
 it have to call retro on the retroed range to do that. I do 
 agree however that
 in general, it doesn't make sense to access source.


Yes, accessing the original range is useful, but AFAIK, container 
doesn't use retro in any way. It does it with take (which also 
has a source field). For take, there is no way to extract the 
source other than with a specialized function.

regarding retro, I find it silly it has a "source" field at all, 
when the original could just be retrieved using retro again (and 
just as efficiently). I don't see any way using source over retro 
could be useful to anyone at all, except for actually 
implementing retro().retro() itself (in which case a _source 
would have done just as well).


 As for


 The problem though is in the way the documentation "The 
 original range can be accessed by using the source property" 
 and "Applying retro twice to the same range yields the 
 original range": Looks like we forgot to notice these two 
 sentences are contradicting.


 I don't see anything contradictory at all. If you call retro on 
 a retroed
 range, you get the original. If you access source, you get the 
 original.
 Where's the contradiction?


In "If you access source, you get the original". This is only 
true if the "The original" is not itslef already a retro. This 
mind sound silly, but you may not have control of this. For 
example,

For example, imagine you want a find function that searches from 
the end, and returns the result of everything before the match:

//----
import std.stdio;
import std.algorithm;
import std.range;

auto findBefore(R)(R r, int n)
{
     auto a = r.retro();
     auto result = a.find(n);
     static assert(is(typeof(result) == typeof(r.retro()))); 
//result is a retro
     return result.source; //Error: undefined identifier 'source'
}

void main()
{
     auto a = [0, 1, 2, 3, 4, 5];
     a.findBefore(3).writeln(); //expect [1, 2, 3]
     a.retro().findBefore(3).writeln(); //expect [5, 4, 3]

     auto b = indexed([2, 1, 3, 0, 4, 5], [3, 1, 0, 2, 4, 5]);
     assert(b.equal(a)); // b is [0, 1, 2, 3, 4, 5]
     b.retro().findBefore(3).writeln(); // produces [2, 1, 3, 0, 
4, 5]...
}
//----
In "findBefore3", one should expect getting back the "original 
range", but that is not the case. However, using "return 
result.retro()"; works quite well.

Things get even stranger if the underlying range also as a 
"source" filed itself (because of the auto return type).

Both the issues can be fixed with "return result.source;"

--------
I don't think there is anything to be gained using "source". I 
don't think it would be worth deprecating it either, and it is 
not a huge issue, but I think the documentation should favor the 
use of double retro over source, or even mention source at all.

Less surprises is always better.
Nov 08 2012
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, November 08, 2012 10:56:38 monarch_dodra wrote:

 On Thursday, 8 November 2012 at 09:18:54 UTC, Jonathan M Davis
 
 wrote:

 In the case of retro, I think that it would good to have source
 exposed for
 std.container's use. It's easy for std.container to understand
 what retro's
 supposed to do, and use it accordingly, and I think that it
 would be silly for
 it have to call retro on the retroed range to do that. I do
 agree however that
 in general, it doesn't make sense to access source.


 
 Yes, accessing the original range is useful, but AFAIK, container
 doesn't use retro in any way. It does it with take (which also
 has a source field). For take, there is no way to extract the
 source other than with a specialized function.


std.container doesn't use retro right now, but it really should (though that 
would require externalizing retro's return type). For instance, what would you 
do if you had to remove the last five elemets from a DList? You can't simply 
take the last 5 and pass that to remove with something like take(retro(list[], 
5)) or retro(take(retro(list[], 5))), because the resulting type is 
unrecognized by remove. You're forced to do something like

list.remove(popFrontN(list[], walkLength(list[]) - 5));

which is highly inefficient.


 regarding retro, I find it silly it has a "source" field at all,
 when the original could just be retrieved using retro again (and
 just as efficiently). I don't see any way using source over retro
 could be useful to anyone at all, except for actually
 implementing retro().retro() itself (in which case a _source
 would have done just as well).


Andrei has expressed interest in having _all_ ranges (or at least a sizeable 
number of them) expose source. That being the case, using retro to get at the 
original doesn't really make sense. That's what source is for. retro naturally 
returns the original type when you retro it again, because it avoids type 
proliferation, but that's specific to retro.

Now, how useful source will ultimately be in generic code which doesn't know 
exactly what range type it's dealing with, I don't know. It may ultimately be 
pretty much useless. But even if you have to know what the type is to use it 
appopriately, since they'd generally be exposing the original range through 
source, it makes sense that retro would do the same.


 As for
 

 The problem though is in the way the documentation "The
 original range can be accessed by using the source property"
 and "Applying retro twice to the same range yields the
 original range": Looks like we forgot to notice these two
 sentences are contradicting.


 
 I don't see anything contradictory at all. If you call retro on
 a retroed
 range, you get the original. If you access source, you get the
 original.
 Where's the contradiction?


 
 In "If you access source, you get the original". This is only
 true if the "The original" is not itslef already a retro. This
 mind sound silly, but you may not have control of this. For
 example,


It makes perfect sense that retro would return the same type when retro is 
called on an already retroed range. And that's the _only_ case where source 
wouldn't exist. The real problem with relying on source from the type returned 
by retro is the fact the result could be _another_ range which exposes source 
rather than retro - e.g. retro(retro(take(range, 5))). The docs aren't really 
incorrect in either case. It's just that in that one case, the result is a 
different type than the rest.

I have no problem with removing mention of source from the docs. I don't think 
that it should be used normally. I do think that it makes sense to have it, 
but it makes sense primarily as part of the general approach of giving ranges 
source members. Certainly, using source directly after a call to retro makes 
no sense. For using source to _ever_ make sense (in general, not just with 
retro), you need to know that you're dealing with a wrapper range. So, using 
source immediately after having called a function which potentially returns a 
wrapper range doesn't really make sense. It makes sense when you already know 
that you're dealing with a wrapper range (e.g. you already know that it's a 
Take!Whatever), and even then, I think that it primarily makes sense when 
you're looking for a specific type that's being wrapped (as is the case with 
std.container) rather than when dealing with a generic type which was wrapped.

- Jonathan M Davis
Nov 08 2012
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 8 November 2012 at 18:20:31 UTC, Jonathan M Davis 
wrote:

 On Thursday, November 08, 2012 10:56:38 monarch_dodra wrote:

 On Thursday, 8 November 2012 at 09:18:54 UTC, Jonathan M Davis
 
 wrote:

 In the case of retro, I think that it would good to have 
 source
 exposed for
 std.container's use. It's easy for std.container to 
 understand
 what retro's
 supposed to do, and use it accordingly, and I think that it
 would be silly for
 it have to call retro on the retroed range to do that. I do
 agree however that
 in general, it doesn't make sense to access source.


 
 Yes, accessing the original range is useful, but AFAIK, 
 container
 doesn't use retro in any way. It does it with take (which also
 has a source field). For take, there is no way to extract the
 source other than with a specialized function.


 std.container doesn't use retro right now, but it really should 
 (though that
 would require externalizing retro's return type). For instance, 
 what would you
 do if you had to remove the last five elemets from a DList? You 
 can't simply
 take the last 5 and pass that to remove with something like 
 take(retro(list[],
 5)) or retro(take(retro(list[], 5))), because the resulting 
 type is
 unrecognized by remove. You're forced to do something like

 list.remove(popFrontN(list[], walkLength(list[]) - 5));

 which is highly inefficient.


It's funny you bring this up, because I've been wrapping my head 
around this very problem for the last week. The root of the 
problem is that Bidirectional ranges (as *convenient* as they 
are) just don't have as much functionality as iterators or 
cursors. If you've used DList more than 5 minutes, you know what 
I'm talking about.

The "retro" trick you mention could indeed be a convenient 
mechanic. (Keep in mind that take is forward range, so can't be 
retro'ed though).

I'm just afraid of what it would really mean to interface with a 
retro'ed range: What exactly does that range represent for the 
original container?

What we'd *really* need (IMO) is a takeBack!Range, that would 
only implement back/popBack. No, the resulting range wouldn't 
*actually* be a range (since it wouldn't have a front), but it 
would still be incredibly useful even if *just* to interface with 
containers, eg:

list.remove(list[].takeBack(5)); //Fine, that's not a "real" 
range, but I know how to interface with that.

I'll toy around with this in my CDList implementation.

----
PS: The problem you bring up is common to all bidirRanges, and 
not just containers: As a general  rule, there is no way to 
extract a subrange from the end of a bidirrange...


 [SNIP]*The rest*[/SNIP]

 - Jonathan M Davis


Read and acknowledged.
Nov 08 2012
prev sibling parent "Mehrdad" <wfunction hotmail.com> writes:
Isn't this easy to fix?

Just make sure typeof(retro(r)) == typeof(retro(retro(retro(r))))

so that you always get back a retro'd range.
Nov 08 2012