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

fix: assign "m_end" as UpperBound for an array of character #3835

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

gxyd
Copy link
Contributor

@gxyd gxyd commented Apr 9, 2024

Fixes #3039

@gxyd gxyd changed the title assign "m_end" as UpperBound when an array of character fix: assign "m_end" as UpperBound for an array of character Apr 9, 2024
tests/tests.toml Outdated Show resolved Hide resolved
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.

Left a comment above otherwise this looks good. Thanks!

@certik
Copy link
Contributor

certik commented Apr 9, 2024

Does the test in this PR fail in master? If so, then just adding the test I think is fine.

The problem with the ASR test is that it doesn't check that ArrayBound is actually there. So we are just hoping that future changes will not break it (that we will spot the regression in the updated reference results diff). While the integration test is clear: if it failed before and now it doesn't, then it must be working (one way or the other), and it will never break with future PRs.

The ASR tests were instrumental to bootstrap the compiler, since we couldn't compile many of the features. But now in the late stages of the compiler, the ASR tests are not as useful.

@@ -1376,6 +1376,7 @@ RUN(NAME character_01 LABELS gfortran llvm)
RUN(NAME character_02 LABELS gfortran llvm)
RUN(NAME character_03 LABELS gfortran llvm)
RUN(NAME character_04 LABELS gfortran llvm)
RUN(NAME char_array_01 LABELS gfortran llvm)
Copy link
Contributor Author

@gxyd gxyd Apr 9, 2024

Choose a reason for hiding this comment

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

I actually made it a part of integration tests and also saved it's ASR as well, see: https://github.com/lfortran/lfortran/pull/3835/files#diff-1ab5e0955089e3094bab05294193ebb1d6f463e1739a7d0b0db2d3297f5a99c8R3881-R3883.

The ASR tests were instrumental to bootstrap the compiler, since we couldn't compile many of the features. But now in the late stages of the compiler, the ASR tests are not as useful.

I still personally would've liked to keep the ASR test, but I think you know things better, so I'm going to follow that 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep it.

@gxyd
Copy link
Contributor Author

gxyd commented Apr 9, 2024

Does the test in this PR fail in master?

It does.

char_arr2(:) = " "

print *, char_arr1
print *, char_arr2
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's test actual values using if(....) error stop conditions.

@gxyd
Copy link
Contributor Author

gxyd commented Apr 10, 2024

We've two test files now:

  • one to save the ASR, which doesn't have any if ... error stop conditions in it and is relatively simple
  • the second one to make sure that the executable output is correct (and has if ... error stop conditions), and on which the current main would fail

@Pranavchiku Pranavchiku merged commit eb540f4 into lfortran:main Apr 10, 2024
21 checks passed
@gxyd gxyd deleted the array_char branch April 10, 2024 11:45
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.

Failure while compiling code with character array
3 participants