Skip to content

ASR: Support param access in, out, inout #1820

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

Merged
merged 5 commits into from
May 17, 2023

Conversation

ubaidsk
Copy link
Collaborator

@ubaidsk ubaidsk commented May 17, 2023

towards #1807.

@ubaidsk ubaidsk marked this pull request as draft May 17, 2023 04:01
@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 17, 2023

I will update the commit history. I would like to request a quick review on this. We also have #1803 which prints error on assignment to intent in parameters. I think/guess we can rebase it (#1803) after merging/completion of this PR for further error handling.

@ubaidsk ubaidsk requested a review from certik May 17, 2023 04:05
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.

Otherwise I think this looks good, thanks!

@certik
Copy link
Contributor

certik commented May 17, 2023

We should also add regular integration tests and CPython tests for this.

@ubaidsk ubaidsk force-pushed the param_access_in_inout_out branch from 9402ab1 to e35f45e Compare May 17, 2023 05:03
@ubaidsk ubaidsk changed the title Param access in inout out ASR: Support param access in, out, inout May 17, 2023
@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 17, 2023

We should also add regular integration tests and CPython tests for this.

Done.

@ubaidsk ubaidsk marked this pull request as ready for review May 17, 2023 05:33
@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 17, 2023

I am adding this to auto-merge as it seems approved. I will work on further modications (if any) in another PR.

@ubaidsk ubaidsk enabled auto-merge May 17, 2023 05:36
@ubaidsk ubaidsk disabled auto-merge May 17, 2023 05:38
Comment on lines +88 to +97
class Intent:
def __init__(self, type):
self._type = type

def __getitem__(self, params):
return params

In = Intent("In")
Out = Intent("Out")
InOut = Intent("InOut")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I disabled the auto-merge. I think/guess this needs a review. I would like to know if this approach is fine for supporting In, Out, InOut in CPython.

@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 17, 2023

Ready.

@ubaidsk ubaidsk requested a review from certik May 17, 2023 05:41

print(a, b, c, d.p)

f(a, b, c, d)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about testing In[list[u32]], In[Foo]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$ cat examples/expr2.py 
from lpython import i32, u32, f64, dataclass, In, Out, InOut

@dataclass
class Foo:
    p: i32

def f(z: In[list[u32]], w: In[Foo]):
    print(z, w.p)


def main0():
    c: list[u32] = [u32(1), u32(2), u32(3), u32(4)]
    d: Foo = Foo(25)

    f(c, d)

    assert c[-1] == u32(4)
    assert d.p == 25

main0()
$ python examples/expr2.py 
[1, 2, 3, 4] 25
$ lpython examples/expr2.py
[1, 2, 3, 4] 25

It seems to work locally. Shall I add this as an integration_test?

Do you mean about printing errors when there is assignment/modification to In variables? I am hoping to handle printing errors in PR #1803 after completion/merging of this PR.

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 this looks good, thanks @Shaikh-Ubaid !

@certik certik merged commit 31ce17a into lcompilers:main May 17, 2023
@ubaidsk ubaidsk deleted the param_access_in_inout_out branch May 17, 2023 19:44
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