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

add support for do concurrent - 2nd try (closes #403) #423

Merged
merged 6 commits into from
Sep 12, 2023

Conversation

rupertford
Copy link
Collaborator

The f2008 do concurrent functionality has already been added but does not get called as the original f2003 classes get called directly from other f2003 classes. In this PR, 2008 versions of these other f2003 classes are added so all should be well.

@rupertford rupertford added in progress F2008 Relates to support for the Fortran 2008 standard labels Jul 30, 2023
@rupertford rupertford self-assigned this Jul 30, 2023
@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (38364a9) 91.77% compared to head (b109852) 91.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   91.77%   91.81%   +0.03%     
==========================================
  Files          38       43       +5     
  Lines       13415    13470      +55     
==========================================
+ Hits        12312    12367      +55     
  Misses       1103     1103              
Files Changed Coverage Δ
src/fparser/two/Fortran2003.py 94.43% <100.00%> (+0.01%) ⬆️
src/fparser/two/Fortran2008/__init__.py 100.00% <100.00%> (ø)
...r/two/Fortran2008/action_term_do_construct_r824.py 100.00% <100.00%> (ø)
...two/Fortran2008/block_label_do_construct_r814_1.py 100.00% <100.00%> (ø)
.../Fortran2008/block_nonlabel_do_construct_r814_2.py 100.00% <100.00%> (ø)
src/fparser/two/Fortran2008/label_do_stmt_r816.py 100.00% <100.00%> (ø)
...c/fparser/two/Fortran2008/nonlabel_do_stmt_r817.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rupertford
Copy link
Collaborator Author

Ready for first review from @arporter or @sergisiso

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Nicely done.
There are a few pylint things to clean-up and I think we could do with one or more slightly more 'functional' tests where we check that 'do concurrent' is parsed correctly in the context of a whole routine - i.e. as a child of a routine, and if-block, another loop...

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Aug 24, 2023
@rupertford
Copy link
Collaborator Author

Ready for next review from @arporter

@rupertford rupertford added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Aug 27, 2023
@rupertford
Copy link
Collaborator Author

Perhaps the rule should be that any classes that simply subclass another class in a different API so that subsequent classes use the the new API, should have functional tests? I think that would then cover all cases and more. I've not done this in this PR but would be happy to do so if the reviewer thinks this is a better solution than the single functional test I've added for one case when using concurrent.

@arporter
Copy link
Member

arporter commented Sep 8, 2023

Perhaps the rule should be that any classes that simply subclass another class in a different API so that subsequent classes use the the new API, should have functional tests? I think that would then cover all cases and more. I've not done this in this PR but would be happy to do so if the reviewer thinks this is a better solution than the single functional test I've added for one case when using concurrent.

Something like that, yes. (I don't quite follow what you mean.) In this case I'd be happier if we at least check for the case where the DO CONCURRENT is a child of:

  • a normal DO loop
  • an IF block
  • a SELECT CASE

Copy link
Member

@arporter arporter 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 tidying up. Almost there now. There appears to be one change that you've not committed and I would like the tests extended a bit.

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Sep 8, 2023
@rupertford
Copy link
Collaborator Author

I've added functional tests where the modified classes are purely there to connect the f2003 to the f2008 concurrent classes. I've just added basic functional tests as everything else will work fine, the important thing is that the different flavours of f2008 concurrent can be parsed from f2003 classes (I've just used program).

Ready for next review from @arporter

@rupertford rupertford added ready for review and removed reviewed with actions PR has been reviewed and is back with developer in progress labels Sep 11, 2023
Copy link
Member

@arporter arporter 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 making those changes.
All good now.
Will proceed to merge.

@arporter arporter merged commit 0d20a0d into master Sep 12, 2023
@arporter arporter deleted the 403_do_concurrent branch September 12, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F2008 Relates to support for the Fortran 2008 standard under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants