-
Notifications
You must be signed in to change notification settings - Fork 32
(Closes #483) Recognize generic directives #486
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #486 +/- ##
=======================================
Coverage 92.19% 92.19%
=======================================
Files 86 86
Lines 13802 13802
=======================================
Hits 12725 12725
Misses 1077 1077 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sergisiso This is ready for a look - small change to support generic directives as discussed yesterday. |
sergisiso
left a comment
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.
Thanks @LonelyCat124 see inline mostly questions and request for test regarding this.
|
@sergisiso Back to you, should be fixed up. |
|
We still get a bit weird locality here: comes from This makes life a little tricky in PSyclone. Also the first one after the declarations is hard to find. |
sergisiso
left a comment
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.
@LonelyCat124 This is better but I still have some comments about the tests.
We still get a bit weird locality here
I think that's alright for now. I think we should focus first in that "keep_comments" and "keep_directives" work, and we can improve the specifics latter.
… 483_generic_directives
|
@sergisiso I think I've tidied up the tests now. The remaining question is whether we do any further work on multi-line directives, but I think that might be difficult for me to do without really diving deep into fparser (and maybe require some significant rewriting of how we handle these now, making comment/directives very different). Maybe its best to just add notes into the documentation for now? |
sergisiso
left a comment
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.
@LonelyCat124 I am still confused about the behaviour, it would be good to complete stfc/PSyclone#3196 so we can see the whole thing.
|
@sergisiso I added the additional tests here. I didn't add a fixed-form acc test though I can add it if you think its worth testing separately. |
sergisiso
left a comment
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.
@LonelyCat124 Ok, I am satisfied with the fparser part of directives, the remaining part is for psyclone to fix. This is approved for merging.
No description provided.