Skip to content

Comments

Using basic_new_heap for assigning elements of symbolic list#2465

Merged
certik merged 2 commits intolcompilers:mainfrom
anutosh491:symbolics_15
Jan 31, 2024
Merged

Using basic_new_heap for assigning elements of symbolic list#2465
certik merged 2 commits intolcompilers:mainfrom
anutosh491:symbolics_15

Conversation

@anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Jan 30, 2024

This PR adds an example to address the first TODO as pointed out here (#2450 (comment))

We are trying to replicate this code snippet essentially

from lpython import S
from sympy import Symbol, pi

def mmrv(r: Out[list[S]]) -> None:
    x: S = pi
    l1: list[S]
    l1 = [x]
    print(l1[0])
    r = l1

def test_mrv2():
    ans: list[S]
    temp: list[S]
    mmrv(ans)
    temp = ans
    print(temp[0])

test_mrv2()

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Jan 30, 2024

Also talking about the note which added to the mentioned comment
Note: in LPython, list assignment r = l1 is always a deep copy (unlike in CPython where it is just a refcount increase), so we should initialize r first using basic_new_heap and then use basic_assign to assign the elements. That can be done later.

I see that this is correct and works without flaws, hence for now the line r = l1 can be replicated using r = l1 itself but in future we might prefer the approach pointed above.

certik
certik approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think that this looks good.

@certik
Copy link
Contributor

certik commented Jan 31, 2024

Is this ready to be merged?

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Jan 31, 2024

Is this ready to be merged?

I think it is ready !

@certik certik merged commit 467081e into lcompilers:main Jan 31, 2024
@certik
Copy link
Contributor

certik commented Jan 31, 2024 •
edited
Loading

The CI failure is due to #2455.

@certik certik mentioned this pull request Jan 31, 2024
Closed
@anutosh491 anutosh491 mentioned this pull request Feb 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@certik certik certik approved these changes

Assignees

No one assigned

Labels

symbolic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anutosh491 @certik