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.

Sign up for GitHub

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

Jump to bottom

Sync libasr with LPython #2837

Merged
merged 3 commits into from
Nov 11, 2023
Merged

Sync libasr with LPython #2837

merged 3 commits into from
Nov 11, 2023

Conversation

czgdp1807
Copy link
Member

@Shaikh-Ubaid
Copy link
Member

@czgdp1807 The WAT tests are good except two reference tests about bindC in wasm. I think these fail because in LPython we updated the wasm backend to use bindJS instead of bindC. I would ideally fix this in a separate PR.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

It seems you are still fixing the sync here, ping me once this is ready.

src/libasr/ASR.asdl Outdated Show resolved Hide resolved
src/libasr/ASR.asdl Outdated Show resolved Hide resolved
src/libasr/runtime/lfortran_intrinsics.c
for (size_t i = 0; i < strlen(*x); i ++) {
*x = (char*) malloc((strlen(y) + 1) * sizeof(char));
_lfortran_string_init(strlen(y) + 1, *x);
for (size_t i = 0; i < strlen(*x); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

TBR

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this change is needed for LPython.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some tests in LPython fail because the logic is incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, this seems to be related to Strings, so I will work on this in a new PR.

Copy link
Member

Choose a reason for hiding this comment

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

Please create an issue so we do not forget.

@czgdp1807
Copy link
Member Author

I would ideally fix this in a separate PR.

For now, what should I do with stderr reference tests generated?

@Shaikh-Ubaid Shaikh-Ubaid mentioned this pull request Nov 10, 2023
Closed
@Shaikh-Ubaid
Copy link
Member

For now, what should I do with stderr reference tests generated?

I am unsure if we are allowed to commit these to main branch, but I created an issue for it #2839.

@Shaikh-Ubaid
Copy link
Member

I am unsure if we are allowed to commit these to main branch, but I created an issue for it #2839.

Since it is a sync PR, I think it is good to support the above issue in a separate PR.

@czgdp1807 czgdp1807 marked this pull request as ready for review November 10, 2023 17:36
@czgdp1807 czgdp1807 mentioned this pull request Nov 10, 2023
Merged
certik
certik reviewed Nov 11, 2023
View reviewed changes
integration_tests/CMakeLists.txt
@@ -938,7 +938,7 @@ RUN(NAME bits_04 LABELS gfortran llvm)
RUN(NAME bits_05 LABELS gfortran llvm)
RUN(NAME bits_06 LABELS gfortran llvm fortran)
RUN(NAME cpu_time_01 LABELS gfortran llvm)
RUN(NAME cpu_time_02_wasm LABELS wasm)
# RUN(NAME cpu_time_02_wasm LABELS wasm)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is going on in this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shaikh-Ubaid will fix this in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it's this: #2839

@certik
Copy link
Contributor

certik commented Nov 11, 2023

For now, what should I do with stderr reference tests generated?

I can't see any stderr tests? Or was this already fixed.

certik
certik approved these changes Nov 11, 2023
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 this looks good, I don't see any blocker here.

Pranavchiku
Pranavchiku approved these changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@Pranavchiku Pranavchiku left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!

Shaikh-Ubaid
Shaikh-Ubaid approved these changes Nov 11, 2023
View reviewed changes
Copy link
Member

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

Thanks for this. It looks good to me!

@czgdp1807 czgdp1807 merged commit 1c90bc4 into lfortran:main Nov 11, 2023
20 checks passed
@czgdp1807 czgdp1807 deleted the libasr_sync_05 branch November 11, 2023 18:42
@czgdp1807 czgdp1807 mentioned this pull request Nov 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thirumalai-Shaktivel Thirumalai-Shaktivel Thirumalai-Shaktivel left review comments

@certik certik certik approved these changes

@Shaikh-Ubaid Shaikh-Ubaid Shaikh-Ubaid approved these changes

@Pranavchiku Pranavchiku Pranavchiku approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@czgdp1807 @Shaikh-Ubaid @certik @Thirumalai-Shaktivel @Pranavchiku