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 inline definition of access rights for Fortran types #15844

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

dcaliste
Copy link
Contributor

@dcaliste dcaliste commented Mar 27, 2020

Allow to parse type definition with inline access specifier, like:
type, public :: foo
end type foo

This is allowed in Fortran90, see for instance https://software.intel.com/en-us/fortran-compiler-developer-guide-and-reference-type-statement-derived-types
Before this MR, only:

 type foo
 end type foo
 public :: foo

is understood by crackfortran.

Copy link
Member

@anirudh2290 anirudh2290 left a comment

Choose a reason for hiding this comment

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

Overall, my concerns are with respect to failure scenarios caused by this new enhancement when the private/public access specifier is not provided.

Also, I am new to numpy codebase and not very familiar with fortran (or f2py), maybe a maintainer with more experience with f2py can take a look.

numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
Copy link
Member

@anirudh2290 anirudh2290 left a comment

Choose a reason for hiding this comment

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

Overall, LGTM from my side. Maybe someone with more experience with fortran can take a look.

cc @melissawm

@melissawm
Copy link
Member

The tests pass but I'm hesitant to approve this as user-defined types are, in general, not supported by f2py. Maybe @pearu can weigh in on this?

@seberg
Copy link
Member

seberg commented May 15, 2020

@pearu do you have an opinion on this (I guess user-defined types in f2py?), I am not sure whether we want this or not right now...

@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

@pearu can we get your feedback on this? Thanks!

@dcaliste
Copy link
Contributor Author

I've rebased the PR on main branch and added a release note.

@pearu pearu self-requested a review August 11, 2021 16:10
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.

Thanks, @dcaliste for this PR! This looks good.

My review suggests some improvements to parsing derived type statements as well as exposing other attributes because we'll need these in subsequent work for supporting derived types in general.

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/crackfortran.py Show resolved Hide resolved
numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
doc/release/upcoming_changes/15844.new_feature.rst Outdated Show resolved Hide resolved
numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved
@dcaliste
Copy link
Contributor Author

dcaliste commented Aug 18, 2021

Thanks a lot @pearu for your detailed review and accurate suggestions. I've updated the PR accordingly, and rebased on latest main in the process.

@dcaliste
Copy link
Contributor Author

@pearu , I've rebased on latest main and shorten a bit some lines to make the linter happy.

@melissawm
Copy link
Member

Hi @pearu - gentle ping to take a final look :) Thanks!

HaoZeke added a commit to HaoZeke/f2py_skel that referenced this pull request Dec 30, 2021
Backported from numpy/numpy#15844

Co-authored-by: Damien Caliste <dcaliste@free.fr>
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 otherwise, and should be merged later today :)

numpy/f2py/tests/test_crackfortran.py Outdated Show resolved Hide resolved
Allow to parse type definition with inline acess specifier, like:
 type, public :: foo
 end type foo
HaoZeke added a commit to HaoZeke/f2py_skel that referenced this pull request Jan 8, 2022
Backported from numpy/numpy#15844

Co-authored-by: Damien Caliste <dcaliste@free.fr>
@melissawm
Copy link
Member

Ok - let's give this a go. Thanks @dcaliste for the patience and all the reviewers for their work here!

@melissawm melissawm merged commit 3816849 into numpy:main Jan 17, 2022
f2py core automation moved this from In progress to Done Jan 17, 2022
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

8 participants