lcompilers / lpython

Python compiler
https://lpython.org/
Other
1.5k stars 158 forks source link

Implement `reversed(seq)` #2644

Open kmr-srbh opened 5 months ago

kmr-srbh commented 5 months ago

Working

def fn():
    alphabets: list[str] = ["a", "b", "c", "d", "e"]
    print("Original:", alphabets)

    reversed_alphabets: list[str] = reversed(alphabets)
    print("Reversed:", reversed_alphabets)

fn()
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
Original: ['a', 'b', 'c', 'd', 'e']
Reversed: ['e', 'd', 'c', 'b', 'a']
kmr-srbh commented 5 months ago

@Shaikh-Ubaid I am repurposing the Reverse() utility function for lists to implement this function. Please tell me if it is the correct way to do this. I will then implement a similar utility function for tuples also.

Another way I think is to handle both the cases in one single function. We can call the reverse utility function for lists from within. We will still need one more function for tuples though.

I request you to guide me on this.

kmr-srbh commented 5 months ago

@Shaikh-Ubaid , the tests for cpython fail as the function returns an iterator there. We could do list(reversed(...)), but this does not work in LPython. It treats the function call as it is and throws an error on llvm code generation.

Shaikh-Ubaid commented 5 months ago

The current approach in this PR reverses the original list which we should not be doing. As a result, it fails for the following:

% git diff
diff --git a/integration_tests/test_builtin_reversed.py b/integration_tests/test_builtin_reversed.py
index 51cf3607f..2f38e2e09 100644
--- a/integration_tests/test_builtin_reversed.py
+++ b/integration_tests/test_builtin_reversed.py
@@ -15,6 +15,7 @@ def test_builtin_reversed():
     reversed_numbers: list[i32] = reversed(numbers)
     print(reversed_numbers)
     assert reversed_numbers == [5, 4, 3, 2, 1]
+    assert numbers == [1, 2, 3, 4, 5]

     # list returned through function call
     alphabet_dictionary: dict[str, i32] = {
% lpython integration_tests/test_builtin_reversed.py
['e', 'd', 'c', 'b', 'a']
[5, 4, 3, 2, 1]
AssertionError
kmr-srbh commented 5 months ago

The current approach in this PR reverses the original list which we should not be doing.

Thank you very much for catching this @Shaikh-Ubaid ! I totally missed that I was modifying the original list. I think we can create a new function in asr_to_llvm.cpp and call listreverse() from there for lists. We can create a new list copy and do all the work. Shall we move ahead with the idea? Tuples can be handled similarly.

Shaikh-Ubaid commented 5 months ago

We can create a new list copy and do all the work. Shall we move ahead with the idea? Tuples can be handled similarly.

I think we should not do that. Iterators do not make a copy. They simply work like a pointer which can be used to traverse an object. reversed will return an iterator that will be used to traverse the object in the opposite direction from the other end.

I would not implement reversed at the moment until we have support for iterator. I am not yet sure if we need support for iterator at the ASR level. I asked about it here https://lfortran.zulipchat.com/#narrow/stream/311866-LPython/topic/Iterator/near/432632427.

kmr-srbh commented 5 months ago

I think it is correct to either implement support for an iterator or wait for it. :+1: