Skip to content

Attempt to Impelent list.count() method without generics #1644

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

Closed
wants to merge 2 commits into from

Conversation

gptsarthak
Copy link
Contributor

I read #941 (comment) but soon realized that generics do not seem to be working yet, so this is my attempt to implement list.count() method without generics

I want to know if I am going in the correct direction, and whether this is the correct way of implementing this method. This PR is not ready to merge as it only supports list[i32] yet.

I have not added tests yet, but here is one performed on my branch:

def test_list():
    arr_i32 :list[i32]
    arr_i32 = [6,12,6,18,6]
    res_i32 :i32 = arr_i32.count(6)
    print(res_i32)
    
test_list()
(base) sarthak@pop-os:~/lpython$ python examples/list.py
3
(base) sarthak@pop-os:~/lpython$ lpython examples/list.py
3

@rsaba5612
Copy link

It looks like you're attempting to implement the count() method for lists in LPython without using generics, which is a good idea given that generics aren't working yet. Your approach seems to be correct, since the count() method simply loops over the list and counts the number of times the given element appears.

However, it's important to note that your implementation currently only supports lists of i32 type. In order to fully implement the count() method for any type of list, you'll need to add support for all the built-in types, such as i64, f32, f64, bool, str, etc.

Also, as you mentioned, tests are missing from your implementation. It's important to add thorough tests for all edge cases and potential errors to ensure that your implementation is correct and robust.

Comment on lines +746 to +755
@overload
def _lpython_list_count(l: list[i32], x: i32) -> i32:
count : i32
count = 0
i : i32
i = 0
for i in l:
if(i == x):
count = count + 1
return count
Copy link
Collaborator

@czgdp1807 czgdp1807 Mar 30, 2023

Choose a reason for hiding this comment

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

Well three approaches,

  1. Use generics - Very messy and not worth it at the moment.
  2. Use IntrinsicFunction infrastructure to programmatically generate the code for these functions and call it in intrinsic_functions.cpp pass.
  3. Insert a new node, ListCount(expr list, expr x) in ASR.asdl and implement it directly in LLVM backend.

Approach 3 is most doable right now and will generate the most efficient LLVM code. However to scale well across all backends, approach 2 is the best one but should be done later. There is no disadvantage in going for approach 3. Once approach 2 is implemented we can remove ListCount node easily. @certik What do you say?

P.S. @gptsarthak I would say don't worry much about my comment right now. Let's see what @certik has to say on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, 3 is fine, later we'll move to 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gptsarthak Go ahead with approach 3 for now.

@czgdp1807 czgdp1807 marked this pull request as draft March 31, 2023 05:06
@czgdp1807
Copy link
Collaborator

Closing in favour of #1676

@czgdp1807 czgdp1807 closed this Apr 7, 2023
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.

4 participants