(#Closes 468) Added Directive node separated from comments #469
Conversation
|
I've realised that this only works with 3.10 and above due to type declarations. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #469 +/- ##
==========================================
+ Coverage 92.15% 92.16% +0.01%
==========================================
Files 86 86
Lines 13734 13765 +31
==========================================
+ Hits 12656 12687 +31
Misses 1078 1078 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Pushed some changes now to fix some docs issues and coverage updates. |
There was a problem hiding this comment.
Thanks Aidan. Everything looks sensible but I do have a couple questions about inheritance and valid locations for directives.
In fact, now that I write that, please could you add a new section to the documentation - probably just before https://fparser.readthedocs.io/en/latest/fparser2.html#preprocessing-directives.
I'm wondering whether it would be wise for us to make this behaviour configurable somehow, just in case interpreting things as directives causes problems for downstream applications. I guess we could do this in the same way as we do for the support for non-standard extensions.
src/fparser/two/Fortran2003.py
Outdated
| # | ||
| # SECTION 2 | ||
| # | ||
| class Directive(Base): | ||
| """ | ||
| Represents a Directive. |
There was a problem hiding this comment.
Admittedly you've only copied what's done for Comment but this is a bit terse. Perhaps say that it's a leaf in the parse tree and contains a single item consisting of the whole directive (assuming I've got that right - I'm just guessing).
src/fparser/two/Fortran2003.py
Outdated
| obj = object.__new__(cls) | ||
| obj.init(string) | ||
| return obj | ||
| elif isinstance(string, FortranReaderBase): |
There was a problem hiding this comment.
pylint will say that quite a few of these elif and else are unnecessary because they are immediately preceded by a return.
There was a problem hiding this comment.
Tidied this up for the directive.
| """ | ||
| Initialise this Directive | ||
|
|
||
| :param comment: The comment object produced by the reader |
There was a problem hiding this comment.
s/comment/directive/ in the docstring, argument and body? (Although type of the comment argument is Comment?)
There was a problem hiding this comment.
Yeah, because the input is a comment I kept this as "comment". I added a bit more saying we initialise from a comment object.
| @@ -207,7 +284,8 @@ def match_comment_or_include(reader): | |||
| :py:class:`fparser.two.Fortran2003.Include_Stmt` | |||
|
|
|||
| """ | |||
| obj = Comment(reader) | |||
| obj = Directive(reader) | |||
There was a problem hiding this comment.
The docstring for this method needs to be updated now that it can also return a Directive.
src/fparser/two/Fortran2003.py
Outdated
| """ | ||
| return str(self.items[0]) | ||
|
|
||
| def restore_reader(self, reader) -> None: |
There was a problem hiding this comment.
Unless I'm missing something, this is just a duplication of Base.restore_reader() and can therefore be removed?
There was a problem hiding this comment.
Yeah, removed. Do you want me to remote it from Comment as well?
| # | ||
| # SECTION 2 | ||
| # | ||
| class Directive(Base): |
There was a problem hiding this comment.
I'm wondering: since a directive is a comment, would it make sense to have Directive subclass Comment? Would that win us anything?
There was a problem hiding this comment.
No, I think I dislike that. For parsing, having walk(..., Comment) and walk(..., Directive) return distinct sets makes more sense, especially since a lot of directives "do something" in real codes, wheras comments have no side effects.
| if comment.items[0] != "": | ||
| assert comment.items[0] == "! A comment!" | ||
| comments = comments + 1 | ||
| assert comments == 1 |
There was a problem hiding this comment.
assert len(out) == 1 and then you don't have to count them? Please could you also add a check that !!$ is still a comment.
We also need a separate test with fixed-format source :-(
There was a problem hiding this comment.
No - for some reason fparser makes a bunch of empty comments in various locations in this tree (this behaviour happened before my changes), so I need to count how many non-empty comments there are.
Do directives exist in fixed-format? I've never really written fixed-format source - i guess I need to check what c $ does? But i'm also not sure what its meant to do.
There was a problem hiding this comment.
Directive are not in the standard so each compiler may be different but cdir$ is understood by flang and gfortran, e.g. https://flang.llvm.org/docs/Directives.html#introduction
There was a problem hiding this comment.
I don't currently handle "!dir$" even, I'd assumed it would start !$ - I probably need to fix that then.
| @@ -710,8 +712,8 @@ def match( | |||
| if match_names: | |||
| start_name = obj.get_start_name() | |||
|
|
|||
| # Comments and Include statements are always valid sub-classes | |||
| classes = subclasses + [di.Comment, di.Include_Stmt] | |||
| # Directives, Comments and Include statements are always valid sub-classes | |||
There was a problem hiding this comment.
Is this definitely true? Are there examples of directives that can appear anywhere in a source file? i.e. outside a program or module unit?
There was a problem hiding this comment.
I assumed a directive could in theory appear anywhere a comment could (since it is a comment?). @sergisiso what do you7 think?
There was a problem hiding this comment.
Again, since it is not standard it is difficult to say exactly. But I woudn't complicate things here. I would let Directive be anywhere, as Comments and Include_Stmts, and let the tooling reading the tree do the best effort to validate/interpret them.
arporter
added
reviewed with actions
|
Addressed most of the comments. |
|
@LonelyCat124 I know we talked about that but I forgot. What is the benefit of differentiating in fparser between Comments and Directives? Are you planning to parse the body of the directive in the future? |
@sergisiso yes - its to separate directives from comments to make handling them in PSyclone better in the future (which I plan to continue down that avenue now). It also cleans up even the current implementation I think. |
|
@arporter I think this is cleaned up and ready for another look now. |
LonelyCat124
added
ready for review
and removed
reviewed with actions
Initial implementation of this feature.
Ready for a look from any of @arporter @sergisiso @hiker