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

ENH: add support for operator() in crackfortran. #15006

Merged
merged 1 commit into from
Jan 4, 2022
Merged

Conversation

dcaliste
Copy link
Contributor

Some interface name may contains parenthesis when used
with operator, like:
interface operator(==)
module procedure my_type_equals
end interface operator(==)

Make the end part properly detected, and store also
the operator ('==' in that case) in the name.

Also implement support to list the implemented by in
any interface declaration.

I would like to add some test for the new feature but I don't see how to write them in f2py/tests, some tips may be appreciated.

@dcaliste
Copy link
Contributor Author

Hello, is there some interest in this ? Is it in a bad shape for integration ?

At least, it may be of some interest for Sphinx Fortran for instance which is generating Fortran source code documentation using this crackfortran.py as a tokenizer. Currently, this operator interface is poorly documented.

@mattip
Copy link
Member

mattip commented Dec 14, 2019

For tests, you can see gh-15035 which added tests for new parsing.

I am not a fortran user. Is the usage pattern you wish to support common? Should it be qualified with a version of fortran where support for this was added?

@dcaliste
Copy link
Contributor Author

Thanks for the link, I'll look at it during the week.

I am not a fortran user. Is the usage pattern you wish to support common?

I think the operator overloading is part of the Fortran90 standard, which is now supported by all compilers (we have since Fortran95, 2003 and 2008 standards).

@mattip
Copy link
Member

mattip commented Dec 23, 2019

Indeed, it seems operator overloading is supported in Fortran 90. Here is one reference, there are many available when searching for "spec fortran 90 operator overload". It seems assignment overloading is also supported so this PR still needs:

  • assignment overload parsing
  • tests
  • a release note in doc/release/upcoming_changes

If any of this is not clear, feel free to ask

@dcaliste
Copy link
Contributor Author

@mattip Thanks for looking on this PR. I'm on vacation leave at the moment, but I'll take care of adding the missing bits you mentioned after the 6th of January. Indeed, I forgot the assignment interface, my bad ;D I would have miss it when moving to some other Fortran sources I'm dealing with.

@pearu pearu added this to In progress in f2py core via automation Feb 20, 2021
Base automatically changed from master to main March 4, 2021 02:04
@melissawm
Copy link
Member

Hello @dcaliste - are you interested in picking up work in this PR? Thanks!

@dcaliste
Copy link
Contributor Author

assignment overload parsing
tests
a release note in doc/release/upcoming_changes

Thank you @mattip for the suggestions. I've added the assignment parsing, a test and a release note (and rebased on top of main).

are you interested in picking up work in this PR?

Thank you for reminding me. I've updated the PR taking the suggestions of @mattip into account.

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

Minor grammar fixes aside; this looks good to me; however, since this does not correspond to an f2py feature yet; @dcaliste could you open an issue so this is highlighted? i.e. an issue which points out the lack of support in f2py for operator(). If you could fix the linter; I think this is ready

doc/release/upcoming_changes/15006.new_feature.rst Outdated Show resolved Hide resolved
doc/release/upcoming_changes/15006.new_feature.rst Outdated Show resolved Hide resolved
@dcaliste
Copy link
Contributor Author

Thank you @HaoZeke for your review. I've corrected the grammar mistakes and pleased the linter by breaking long lines.

About creating an issue about the lack of operator() support in f2py, I think, it's closely related to the lack of support for Fortran derived-types, as mentioned by @pearu in issue #14938. Indeed, there is no point to override operator and assignment for basic Fortran types. I may comment there the existence of this PR that may contribute to the effort for Fortran derived-type. Or do you prefer to open a new issue, specifically for this ?

@HaoZeke
Copy link
Member

HaoZeke commented Sep 20, 2021

I think it would be good to have separate issues; so we can track and close them. Thanks for making the changes! I think we should merge this sometime this week.

@dcaliste
Copy link
Contributor Author

Thank you @HaoZeke, I've opened #19896 to follow the addition of operator() and assigment module procedures. I mentioned there that this PR is a step in that direction.

numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
@dcaliste
Copy link
Contributor Author

dcaliste commented Jan 3, 2022

I've corrected the test to include a valid Fortran snippet and also took the opportunity to rebase the PR.

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

Apart from the nit I think this is good to go in.

numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
@dcaliste dcaliste force-pushed the crack branch 2 times, most recently from 609d0ed to 753a146 Compare January 4, 2022 08:16
Some interface name may contains parenthesis when used
with operator, like:
 interface operator(==)
   module procedure my_type_equals
 end interface operator(==)

Make the end part properly detected, and store also
the operator ('==' in that case) in the name.

Also implement support to list the implemented by in
any interface declaration.
Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @dcaliste!

Btw, while reading this PR, I was unsure if crackfortran should be called a parser because it is not a parser in a conventional sense. For instance, crackfortran does not care if the input is syntactically correct or not while conventional parsers do. However, since it builds up an internal structure representing a part of Fortran code that is relevant only to wrapping Fortran to Python, I guess the parser may still be an appropriate term.. at least, I could not come up with a suggestion for a better term for crackfortran.

@charris
Copy link
Member

charris commented Jan 4, 2022

@HaoZeke Want to update your review?

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dcaliste
Copy link
Contributor Author

dcaliste commented Jan 4, 2022

Btw, while reading this PR, I was unsure if crackfortran should be called a parser because it is not a parser in a conventional sense.

May I suggest "analyser" as a replacement ? I'm not a native English speaker, so it may not fit neither...

@charris charris merged commit 09ffcf5 into numpy:main Jan 4, 2022
f2py core automation moved this from In progress to Done Jan 4, 2022
@charris
Copy link
Member

charris commented Jan 4, 2022

Thanks @dcaliste .

@dcaliste dcaliste deleted the crack branch January 4, 2022 20:33
@InessaPawson
Copy link
Member

Hi-five on merging your first pull request to NumPy, @dcaliste! We hope you stick around! Your choices aren’t limited to programming – you can review pull requests, help us stay on top of new and old issues, develop educational material, work on our website, add or improve graphic design, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://numpy.org/contribute/.
Also, consider joining our mailing list. This is a great way to connect with other cool people in our community and be part of important conversations that affect the development of NumPy: https://mail.python.org/mailman/listinfo/numpy-discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants