Fix list.pop() for negative arguments

[faze-geek]

Snippet

def f():
    a :list[i32] = [1, 2, 3, 4, 5]
    print(a.pop(-1))
    print(a.pop(-4))
    print(a)
    
f()

(lp) C:\Users\kunni\lpython>python try.py
5
1
[2, 3, 4]

On master -

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
IndexError: List index is out of range. Index range is (0, 4), but the given index is -1

On branch -

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
5
1
[2, 3, 4]

[faze-geek]

I just went over post 1 of this GSoC blog. The work done in the project is commendable. I will try to make some contributions if I see any possible improvements. Thanks!

[faze-geek]

Been a few days since I pushed the code. Does it look good to you ? @certik

[certik]

Can you please update the equivalent C++ code above, to make it clear how the logic works?

[faze-geek]

Sure, if we are going ahead with the change I will update the logic as well.
For understanding the logic is below -

if(pos<0){
      if(abs(pos)<= end_point){
            adjusted_pos = endpoint + pos
    }
}
else{
   adjusted_pos = pos
}
if(adjusted_pos <0 || adjusted_pos>=end_point) return IndexError

[certik]

For arrays we do not support negative indices (if I am not mistaken), due to runtime overhead that cannot be removed even in Release mode. This PR I think introduces a similar runtime slowdown that cannot be optimized out, correct?

If so, the question to discuss is if we want to introduce such a slowdown in exchange to support negative indices, or should we rather disallow negative indices in exchange for faster runtime.

[faze-geek]

For arrays we do not support negative indices (if I am not mistaken), due to runtime overhead that cannot be removed even in Release mode.

As far as I remember, negative indexing works and gives correct results with lists -

def main0():
    l: list[i32] = [1, 2, 3]
    print(l[-1])
    # I think x : i32 = 2 ; print(l[-x]) is the one you refer to as not working (or totally unwanted).

main0()

~/lpython/lpython$ ./src/bin/lpython examples/expr2.py 
3

If so, the question to discuss is if we want to introduce such a slowdown in exchange to support negative indices, or should we rather disallow negative indices in exchange for faster runtime.

I came across this issue and felt the need for it because if it is not discarded and returning answers, the output might as well be correct (over incorrect).

Stating @czgdp1807 statement from this comment here -
""
The source of confusion in lists is that we are allowing runtime indices for positive values but not for negative values. On top of that compile time indices of any sign are allowed in lists. For compile time case one can use both positive and negative indexing in lists. So that will intuitively make one think that we can also do the same with runtime indices but then we face the out of bounds error.

Even if we are okay with current status in main, but I think users should receive a detailed error message when they use runtime negative indices in lists,

Something like, "Using negative indices with lists at runtime causes performance slowdowns (<probabaly a link to benchmark where slowdown is observed>), hence LPython doesn't allow such use cases".
""

[certik]

I think compile time negative indexing can be implemented, but runtime cannot without an overhead. So we can implement compile time, but not runtime.

[faze-geek]

So we can implement compile time, but not runtime.

I get your point. So should we try to give the correct solution in compile time but give the above mentioned error in runtime cases ?

[certik]

Yes, runtime should give an error in Debug mode (but not Release mode) for the -1 index.

[Thirumalai-Shaktivel]

What is the status of this PR?

[Thirumalai-Shaktivel]

Please mark this PR ready for review once it is ready.

[faze-geek]

Hi Thirumalai, unfortunately I do not recall what was wrong in this PR. The latest comments by certik mention -

I think compile time negative indexing can be implemented, but runtime cannot without an overhead. So we can implement compile time, but not runtime.

Yes, runtime should give an error in Debug mode (but not Release mode) for the -1 index.

Can you help me to figure out what exactly I need to do here? The use case here is correct, so let's try to get this merged!

[faze-geek]

Hi @Shaikh-Ubaid,
Could you have a look and kindly explain me the difference between the compile time case and the runtime case here.

[Shaikh-Ubaid]

I think Ondrej means the following:

• For compile-time negative index:
  • support list.pop() for negative argument
• For run-time negative index:
  • Debug mode: throw error for list.pop() when lpython is compiled in debug mode
  • Release mode: let the code segfault for negative index (as it does today for out-of-bounds index), but deliver maximum performance for valid non-negative index

Since we would be checking for negative index in debug mode, it would add a computation overhead. We avoid/tackle this computation overhead by not checking for negative index in release build.

[certik]

Yes, that's exactly what I would do.