Skip to content

[NFC][Fortran/gfortran] Add script to generate static test configuration #91

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 4 commits into from
Feb 15, 2024

Conversation

tarunprabhu
Copy link
Contributor

This adds the script to generate the static test configuration files from the DejaGNU annotations from the test sources. The script is only intended to be used when the test files are updated from upstream.

This adds the script to generate the static test configuration files from the
DejaGNU annotations from the test sources. The script is only intended to be
used when the test files are updated from upstream.
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

When I ran this myself, achar_2.f90 was changed from a run to a compile test. The output complained that it could not open the file. If this cannot be reproduced on your machine then don't let it block the patch.

Otherwise, this looks good. I'm not an expert in python or dejagnu, but the code seems well structured and easy to follow. The FIXMEs seem reasonable to leave for a later date because this can already generate a test configuration that is an improvement upon what we had before.

Thank you for your work on this!

@tarunprabhu tarunprabhu changed the title [NFC][Fortran/gfortran] Add test to generate static test configuration [NFC][Fortran/gfortran] Add script to generate static test configuration Feb 8, 2024
@tarunprabhu
Copy link
Contributor Author

When I ran this myself, achar_2.f90 was changed from a run to a compile test. The output complained that it could not open the file. If this cannot be reproduced on your machine then don't let it block the patch.

Strange. I'll check again on my end.

Thanks for taking a look. I'll leave this up over the weekend to give others a chance to review it.

Copy link
Contributor

@luporl luporl left a comment

Choose a reason for hiding this comment

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

LGTM. I have only a couple nits and suggestions, but feel free to ignore them.

@tarunprabhu
Copy link
Contributor Author

When I ran this myself, achar_2.f90 was changed from a run to a compile test. The output complained that it could not open the file. If this cannot be reproduced on your machine then don't let it block the patch.

Strange. I'll check again on my end.

@tblah, It seems to be working fine for me.

@tarunprabhu
Copy link
Contributor Author

@luporl Thanks for the careful review.

I have reworded some of the comments in the code. Could you take a look and let me know if they are clearer now?

Copy link
Contributor

@luporl luporl left a comment

Choose a reason for hiding this comment

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

@tarunprabhu Thanks for taking the time for addressing my comments, everything is clearer now.

@tarunprabhu tarunprabhu merged commit bc20e74 into llvm:main Feb 15, 2024
@tarunprabhu tarunprabhu deleted the pr-static-test-config-2 branch February 15, 2024 14:39
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