OpenEuphoria

Euphoria Ticket #240: Allow remove() and replace() to accept location indexes relative to the sequence end.

• Reported by DerekParnell Oct 23, 2010
• 12 comments

-- To remove the last three elements 
result = remove(target, length(target) - 2) 

-- To remove the last three elements 
result = remove(target, -2) 

Details

1. Comment by SDPringle Nov 02, 2010

It seems to me it would be a lot more intuitive to make remove( target, -3 ) equivalent to remove( target, length(target) - 2 ) for in the former case we are specifying the number of elements to remove.

remove( target, -1 ) remove the last element remove( target, -2 ) remove the last two remove( target, -3 ) remove the last three

2. Comment by jimcbrown Nov 02, 2010

It's fairly easy to implement this with the following patch. I'm wondering though, if this will break some uses of replace() ?

Index: feu6/source/be_execute.c 
=================================================================== 
--- feu6/source/be_execute.c    (revision 3780) 
+++ feu6/source/be_execute.c    (working copy) 
@@ -3953,6 +3953,8 @@ 
                                if (IS_SEQUENCE(top)) 
                                        RTFatal("Third argument to remove() must be an atom"); 
                                end_pos = (IS_ATOM_INT(top)) ? top : (long)(DBL_PTR(top)->dbl); 
+                               if (end_pos < 1) 
+                                       end_pos+=seqlen; 
                                if (end_pos > seqlen) 
                                        end_pos=seqlen; 
                                obj_ptr = (object_ptr)pc[4]; 
Index: feu6/source/be_runtime.c 
=================================================================== 
--- feu6/source/be_runtime.c    (revision 3780) 
+++ feu6/source/be_runtime.c    (working copy) 
@@ -6040,6 +6040,10 @@ 
        copy_from = *rb->copy_from; 
 
        seqlen = SEQ_PTR( copy_to )->length; 
+       if (end_pos < 1) 
+       { 
+               end_pos += seqlen; 
+       } 
 
        if (end_pos < 0 && start_pos <= seqlen) {  // return (replacement & target) 
                Concat( rb->target, copy_from, copy_to ); 

3. Comment by SDPringle Nov 02, 2010

The markup got messed up in my last post: What I meant to say is:

The way this request is the following would be true:

remove( target, -1 ) -- the request would make this a NOP. 
remove( target, -2 ) -- the request would make this remove the last element. 

Let's change this slightly such that the negative number specifies the number of elements to remove from the end.

remove( target, -1 ) -- removes the last element 
remove( target, -2 ) -- removes the last two elements 

4. Comment by jimcbrown Nov 02, 2010

Shawn, your solution is intuitive and human readable. Negative one removes one element at the end, negative two removes two elements from the end, and so on. Presumably, specifying 0 is then a NOP. My patch implements this.

If I understand Derek, he wants 0 to remove 1 element, negative one to remove two elements, and so on.

Either behavior is easy to implement, so it's just a matter of deciding what the right way to go is.

5. Comment by ArthurCrump Nov 02, 2010

I like Shawn's solution except for the fact that it would be different from the usage in the slice function. I think that could be confusing.

6. Comment by jimcbrown Nov 02, 2010

I agree that all 3 should be made consistent.

7. Comment by mattlewis Nov 02, 2010

The negative notation is good, but remember that the third element defaults to the value of the second, so:

-- To remove the second to last element 
result = remove(target, -2) 
 
-- To remove the last three elements  
result = remove(target, -2, 0)  

8. Comment by DerekParnell Nov 02, 2010

I disagree with the idea that the negative notation should denote a count of items to remove.

Also my original examples were wrong (wish we could edit ticket comments). They should have been ...

-- CURRENT FUNCTIONALITY -- 
-- To remove the third last element 
result = remove(target, length(target) - 2)  
 
-- To remove the last three elements  
result = remove(target, length(target) - 2, length(target))  

-- SUGGESTED FUNCTIONALITY -- 
-- To remove the third last element 
result = remove(target, -2)  
 
-- To remove the last three elements  
result = remove(target, -2, 0)  

The current signature to remove() and replace() use indexes to specify the range of items to affect, so if we start using -ve values as a count it becomes an inconsistency that is more error prone.

My suggestion keeps the idea that the parameters are index references rather than counts.

What I really would have liked to do is use the '$' symbol such as ...

-- To remove the third last element 
result = remove(target, $ - 2)  
-- To remove the last three elements 
result = remove(target, $ - 2, $)  

because in effect the 2nd and 3rd parameters specify a slice of the first parameter. But this might be asking too much of the parser.

If we choose to have a function that uses an element count as a parameter then we should be using a different function. e.g.

-- To remove the third last element 
result = remove_tail(target, 2, 1) -- 2 from last item  for a count of 1 
-- To remove the last three elements 
result = remove(target, 0, 3) -- 0 from last item for a count of 3 

9. Comment by jeremy Nov 02, 2010

When one function does too many things it becomes confusing. I do like that in Derek's examples the parameters are at least the same thing, i.e. indexes.

10. Comment by mattlewis Nov 02, 2010

Yes, I think the best thing is to start using the $ in context where it makes sense. That also prevents accidental negatives due to bugs or bad math. This will require some parsing changes, but I don't think that's such a major issue, since we're slating this for 4.1.

11. Comment by ne1uno Nov 02, 2010

from the front of the string is base 1 but from the back it's base 0 ? that has to be wrong.

12. Comment by SDPringle Nov 02, 2010

See this ticket http://openeuphoria.org/ticket/308.wc

Include: