-
Notifications
You must be signed in to change notification settings - Fork 30
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
#443 Support flag that triggers fparser to read code in OpenMP conditional sentinels #444
Conversation
…ional sentinels.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #444 +/- ##
==========================================
+ Coverage 91.99% 92.07% +0.08%
==========================================
Files 85 85
Lines 13678 13825 +147
==========================================
+ Hits 12583 12730 +147
Misses 1095 1095 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good Joerg, thanks for adding this.
Mostly small things/renaming/extending tests.
Please could you also update the documentation (I've indicated a suitable location somewhere in the review).
…hout optional &.
Ready for next review. |
Hi @hiker, github isn't showing any commits since the last review? |
Dang, yes, I missed that you updated this branch, so my push didn't go through, and I didn't notice. Now something should be there :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there now. Just a few minor things and some weird cases where black has changed a "..." + "..." into "..." "...".
Update doc builds fine.
I've checked on CodeCov and all is well, even if that's not being reported here.
…ng concatenations warnings.
Ready for next review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good now, thanks for those changes.
Will proceed to merge.
Fixed #443, and is important for FAB to detect dependencies in omp sentinels.
The implementation is (unfortunately) on two different levels:
!$omp
(since a '&' and esp. spaces around that are all optional), so it's handled on higher up when combining multiple lines.Moving the fixed format handling up (to the level where the free format is handled) seems to be more difficult, since free format is handled in many different ways when combining lines (strict format, fixed, f77) - so I opted for the two different levels.
I can confirm that this with changes fab now (when supplying the required optional flag) detects dependencies in conditional omp sentinels.