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 f08 do concurrent (closes #403) #408

Merged
merged 11 commits into from
May 15, 2023
Merged

Conversation

rupertford
Copy link
Collaborator

Replaces PR #404 to provide a complete implementation where the matching to do concurrent is limited to f2008 code (f2003 will not match). Also adds appropriate f2008 tests, pulls out and extends the existing f2003 tests and updates the f2003 implementation.

To add in the matching to do concurrent I have changed the tuple that is returned by a match in f2003. This will affect any tools that walk the fparser2 tree structure. The first argument is now a string which specifies the type of match "WHILE" or "COUNTER". In the f2008 match "CONCURRENT" is also allowed. The second argument returns the classes resulting from matching and the third argument returns the optional delimiter. The reason for adding the first argument string is that it helps a tree traversal/transformation tool to determine what to expect in the second argument, as the classes returned are different for the different cases. The previous implementation returned classes in the first argument if matching a WHILE and None in the second and None in the first argument and classes in the second argument if matching a COUNTER, but this does not work well when extending to add CONCURRENT in F08.

@rupertford rupertford added in progress F2008 Relates to support for the Fortran 2008 standard labels Apr 29, 2023
@rupertford rupertford self-assigned this Apr 29, 2023
@rupertford rupertford changed the title 403 do concurrent add support for f08 do concurrent (closes #403) Apr 29, 2023
@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

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

Comparison is base (be09a23) 91.74% compared to head (cf09680) 91.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
+ Coverage   91.74%   91.75%   +0.01%     
==========================================
  Files          37       37              
  Lines       13361    13384      +23     
==========================================
+ Hits        12258    12281      +23     
  Misses       1103     1103              
Impacted Files Coverage Δ
src/fparser/two/Fortran2003.py 94.40% <100.00%> (-0.01%) ⬇️
src/fparser/two/Fortran2008.py 99.49% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rupertford
Copy link
Collaborator Author

I guess an alternative to the first return item having "COUNTER" which is just a made up name and is not part of the Fortran standard is to treat the first argument as a keyword and return None instead of "COUNTER". That would still allow tools that traverse the tree to distinguish. I'll leave it to the reviewer to see is they prefer that option (or some other of course).

@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. Much though I don't like using strings as identifiers (we could have an enum), it is in keeping with the rest of the code so I think it can stay as is.
pylint is happy. Updated doc builds OK.
Just a couple of minor things to address and then this can be merged.

src/fparser/two/Fortran2003.py Outdated Show resolved Hide resolved
src/fparser/two/Fortran2003.py Outdated Show resolved Hide resolved
src/fparser/two/Fortran2008.py Show resolved Hide resolved
src/fparser/two/Fortran2008.py Outdated Show resolved Hide resolved
@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels May 2, 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 May 2, 2023
@arporter
Copy link
Member

@rupertford was this the PR we needed to have a discussion about?

@rupertford
Copy link
Collaborator Author

After discussion with @arporter we have agreed to keep the original order of return values and to add additional return values to the end. This will avoid errors with any tools that use fparser. The best solution, which is to create new classes will be documentated in the docstring and in an issue but will not be implemented until we decide on a suitable time for non-backward compatible changes. A summary of the changes is below:

Backwards compatible
F2003: Loop_Cntl: sle, counter_expr, delim
F2008: Loop_Cntl: sle, counter_expr, delim, (conc_expr)
F2018: Loop_Cntl: sle, counter_expr, delim, (conc_expr, local_x)

F2003: While_Loop_Cntl: sle, delim
F2003: Counter_Loop_Cntl: var, lower, upper, [step], delim
F2008: Concurrent_Loop_Cntl: conc_expr, delim
F2018: Concurrent_Loop_Cntl: conc_expr, local_x, delim

@rupertford
Copy link
Collaborator Author

OK, ready for review again from @arporter

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.

All requested changes have been made. I've taken the liberty of fixing a minor error in a docstring.
Will proceed to merge.

src/fparser/two/Fortran2008.py Outdated Show resolved Hide resolved
@arporter arporter added ready for merge PR is waiting on final CI checks before being merged. and removed under review labels May 15, 2023
@arporter arporter merged commit fbcc2c6 into master May 15, 2023
@arporter arporter deleted the 403_do_concurrent branch May 15, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement F2008 Relates to support for the Fortran 2008 standard ready for merge PR is waiting on final CI checks before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants