Skip to content

LLVM: Fix list.insert() when given index is greater than length of the list #1565

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

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

Conversation

faze-geek
Copy link
Contributor

Consider list.insert(position,element), In python if position is greater than the length of list then by default the element is inserted at position = len(list).
On master -

def f():
    l:list[str] = ["a","b","c","d"]
    l.insert(6,"e")
    print(l)
f()

(lp) faze-geek@DESKTOP-497JDCU:~/lpython/lpython$ python try.py
['a', 'b', 'c', 'd', 'e']
(lp) faze-geek@DESKTOP-497JDCU:~/lpython/lpython$ ./src/bin/lpython try.py
['a', 'b', 'c', 'd', '(null)']    #lpython adds a garbage value

def f():
    l:list[i32] = [1,2,3]
    l.insert(8,4)
    print(l)
f()

(lp) faze-geek@DESKTOP-497JDCU:~/lpython/lpython$ python try.py
[1, 2, 3, 4]
(lp) faze-geek@DESKTOP-497JDCU:~/lpython/lpython$ ./src/bin/lpython try.py
[1, 2, 3, 0]    #lpython adds a garbage value

On branch -

def f():
    l:list[str] = ["a","b","c","d"]
    l.insert(6,"e")
    print(l)
f()

(lp) C:\Users\kunni\lpython>python try.py
['a', 'b', 'c', 'd', 'e']
(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
['a', 'b', 'c', 'd', 'e']

@czgdp1807 czgdp1807 marked this pull request as draft March 7, 2023 17:48
@faze-geek faze-geek requested review from czgdp1807 and Thirumalai-Shaktivel and removed request for czgdp1807 and Thirumalai-Shaktivel March 10, 2023 06:39
@faze-geek faze-geek marked this pull request as ready for review April 9, 2023 07:58
Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

Please add a test for it or point me to any existing tests which verify that your change works correctly.

@faze-geek
Copy link
Contributor Author

Please add a test for it or point me to any existing tests which verify that your change works correctly.

I have added a test case now. Basically, it handles cases when first argument is greater than list length.

@czgdp1807 czgdp1807 marked this pull request as draft April 10, 2023 04:31
@faze-geek faze-geek marked this pull request as ready for review April 13, 2023 04:28
@faze-geek faze-geek requested a review from czgdp1807 April 13, 2023 04:28
@faze-geek
Copy link
Contributor Author

faze-geek commented Apr 14, 2023

@czgdp1807
I investigated the failing logs now. Firstly running the integration_tests/test_list_03.py (where tests have been added) does not give a segmentation error in windows MSVC unlike my WSL setup. This is a seperate issue.
Secondly the c backend in WSL raises the Segmentation Fault when I test indexes greater than the length of the list and as I use an index within the length the error does not occur.
But then this case is a valid edge case as I have shown above, comparing with Cpython -

def test_list_insert_03():
    l1:list[str] = ["a", "b", "c", "d"]
    l1.insert(5,"e")
    s:str = "a"
    i:i32 = 0
    # for i in range(len(l1)):
    #     assert l1[i] == chr(ord(s)+i)
    print(l1)

test_list_insert_03()

(lp) C:\Users\kunni\lpython>python try.py
['a', 'b', 'c', 'd', 'e']

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
['a', 'b', 'c', 'd', 'e'] 

adjusted_pos,
llvm::ConstantInt::get(context, llvm::APInt(32, 0))
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like an issue with edge case. Note that current_end_point equals len(list) all the times.

llvm::Value* LLVMList::len(llvm::Value* list) {
return LLVM::CreateLoad(*builder, get_pointer_to_current_end_point(list));
}

@czgdp1807 czgdp1807 marked this pull request as draft April 15, 2023 15:36
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.

3 participants