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

ASR -> CPython: Add visitors for ListConstant, TupleConstant & SetConstant #2690

Merged

Conversation

kmr-srbh
Copy link
Contributor

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

from lpython import i32, f64

my_first_list: list[i32] = [1, 2, 3 , 4]
print(my_first_list)

my_second_list: list[str] = ["a", "b", "c", "d"]
print(my_second_list)

my_third_list: list[list[i32]] = [[1, 2], [3, 4], [5, 6]]
print(my_third_list)

my_fourth_list: list[list[f64]] = [[1.0, 2.2], [3.6, 4.9], [5.1, 6.3]]
print(my_fourth_list)

my_first_tuple: tuple[i32, str, f64] = (1, "hello", 2.4)
print(my_first_tuple)

my_second_tuple: tuple[tuple[i32, str], str] = ((1, "hello"), "world")
print(my_second_tuple)

my_first_set: set[i32] = {1, 2, 3, 2, 4}
print(my_first_set)

my_second_set: set[f64] = {1.1, 2.5, 6.8}
print(my_second_set)

my_third_set: set[str] = {"a", "b", "a", "c"}
print(my_third_set)
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py --show-python
def __main__global_init():
    my_first_list = [1, 2, 3, 4]
    my_second_list = ["a", "b", "c", "d"]
    my_third_list = [[1, 2], [3, 4], [5, 6]]
    my_fourth_list = [[1.000000, 2.200000], [3.600000, 4.900000], [5.100000, 6.300000]]
    my_first_tuple = (1, "hello", 2.400000)
    my_second_tuple = ((1, "hello"), "world")
    my_first_set = {1, 2, 3, 2, 4}
    my_second_set = {1.100000, 2.500000, 6.800000}
    my_third_set = {"a", "b", "a", "c"}
def __main__global_stmts():
    print(my_first_list)
    print(my_second_list)
    print(my_third_list)
    print(my_fourth_list)
    print(my_first_tuple)
    print(my_second_tuple)
    print(my_first_set)
    print(my_second_set)
    print(my_third_set)

TODO

  • Add tests

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

@Shaikh-Ubaid what are your thoughts on this, is this the right way to do it? Could you please guide me on adding tests for this?

@kmr-srbh kmr-srbh changed the title Add ListConstant visitor for CPython ASR -> CPython: Add ListConstant visitor May 9, 2024
@nikabot
Copy link

nikabot commented May 9, 2024

Regarding tests, I think you enable tests for python in tests.toml and search for list1.py and add python=true.

@nikabot
Copy link

nikabot commented May 9, 2024

And please make sure the generated python code and tests matches the same line by line. I mean, manually verify the output is the same as the test code.

@Shaikh-Ubaid
Copy link
Collaborator

Regarding tests, I think you enable tests for python in tests.toml and search for list1.py and add python=true.

Yes, let's just add one reference test that includes all three list, tuple, set. In the long term, we need an integration_tests approach for testing this.

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

@Shaikh-Ubaid @nikabot since the visitors will be similar for 3 of 4 data-structures, shall I implement them and test them together? I think it would be fast this way. For DictConstant, we can have the separate PR.

@Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid @nikabot since the visitors will be similar for 3 of 4 data-structures, shall I implement them and test them together? I think it would be fast this way. For DictConstant, we can have the separate PR.

Yes.

@kmr-srbh kmr-srbh force-pushed the cpython-list-constant-visitor branch from 913e293 to 1c8a7e6 Compare May 9, 2024 16:35
@kmr-srbh kmr-srbh changed the title ASR -> CPython: Add ListConstant visitor ASR -> CPython: Add visitors for ListConstant, TupleConstant & SetConstant May 9, 2024
@kmr-srbh kmr-srbh marked this pull request as ready for review May 9, 2024 16:58
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

@Shaikh-Ubaid @nikabot what can be a better name for the test file?

@Shaikh-Ubaid
Copy link
Collaborator

what can be a better name for the test file?

test_aggregate_constants or something similar.

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

Generated code verified using CPython interpreter v3.10.13.

@Shaikh-Ubaid
Copy link
Collaborator

Generated code verified using CPython interpreter v3.10.13.

@kmr-srbh the generated code does not work with CPython. There are couple things we will need to fix for it to work.

  • call to __main__global_init() , __main__global_stmts() needs to be added at the end.
  • ensure the variables defined in __main__global_init() are accessbile to __main__global_stmts().
  • and any other issues that we come across while fixing the above two

The above should be tackled in different PR and should not be part of this PR.

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

Generated code verified using CPython interpreter v3.10.13.

@kmr-srbh the generated code does not work with CPython. There are couple things we will need to fix for it to work.

* call to `__main__global_init()` , `__main__global_stmts()` needs to be added at the end.

* ensure the variables defined in `__main__global_init()` are accessbile to `__main__global_stmts()`.

* and any other issues that we come across while fixing the above two

The above should be tackled in different PR and should not be part of this PR.

You are right @Shaikh-Ubaid. I had planned to look into this to get the CPython back-end working. A separate PR will be good! 👍

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 9, 2024

Generated code verified using CPython interpreter v3.10.13.

I tested the code for syntax errors using this.

@Shaikh-Ubaid Shaikh-Ubaid merged commit bdd9ad5 into lcompilers:main May 9, 2024
14 checks passed
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