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

Implement reversed(seq) #2644

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

kmr-srbh
Copy link
Contributor

@kmr-srbh kmr-srbh commented Apr 9, 2024

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
Copy link
Contributor Author

kmr-srbh commented Apr 9, 2024

@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
Copy link
Contributor Author

kmr-srbh commented Apr 9, 2024

@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.

@kmr-srbh kmr-srbh changed the title Implement reversed() Implement reversed(seq) Apr 9, 2024
@@ -19,6 +19,7 @@ the code size.

enum class IntrinsicElementalFunctions : int64_t {
ObjectType,
Reversed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reversed returns an iterator, but does not actually reverse a list. I think we do not have support for iterators yet in LPython.

The approach in this PR reverses the entire list, which I think is not the correct approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shaikh-Ubaid I return a reversed list using the approach used for dict.keys (#1881 (comment)) as we do not support iterators now.

@Shaikh-Ubaid
Copy link
Collaborator

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

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft April 11, 2024 00:42
@kmr-srbh
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

I think it is correct to either implement support for an iterator or wait for it. 👍

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

Successfully merging this pull request may close these issues.

2 participants