Skip to content

BUG: f2py: better handle filtering of public/private subroutines#27049

Merged
HaoZeke merged 2 commits intonumpy:mainfrom
m-weigand:gh26920
Aug 27, 2024
Merged

BUG: f2py: better handle filtering of public/private subroutines#27049
HaoZeke merged 2 commits intonumpy:mainfrom
m-weigand:gh26920

Conversation

@m-weigand
Copy link
Contributor

This should fix gh-26920 and add a corresponding test case.

I am unsure about some formalities and would appreciate some feedback:

  • I am not sure if I put the test in the correct location
  • Technically, this fixes another underlying bug: Handling of f90 modules that have no public variables or subroutines. I'm not sure if this warrants a separate test, but please let me know and I will reformat accordingly.

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jul 26, 2024
@charris charris added this to the 2.0.2 release milestone Jul 26, 2024
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.

I have two minor comments, and beyond that I think it would be nice to have a separate test for the no public subroutine or variable case, otherwise, this looks great.

Comment on lines +115 to +116
if len(var) == 1 and 'attrspec' in var and var['attrspec'][0] in ('public', 'private'):
is_not_var = True
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this entire check could be extracted into a functional form in auxfuncs.py instead. Similar to the existing:

def isprivate(var):
    return 'attrspec' in var and 'private' in var['attrspec']

This is a bit nitpicky and mostly for consistency with the rest of the F2PY code, the form here also works.

Copy link
Member

Choose a reason for hiding this comment

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

Also, what about the protected access property? (not sure how / if this is handled in F2PY now actually).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, the protected property is correctly handled, i.e., using it produced no errors for me. However, I'm not familiar enough with it to tell if I missed anything here.

@charris charris modified the milestones: 2.0.2 release, 2.1.0 release Aug 9, 2024
@EwoutH
Copy link
Contributor

EwoutH commented Aug 10, 2024

It this the last PR that needs to be merged for a 2.1.0 release candidate? (as of the 2.1.0 milestone)

@charris
Copy link
Member

charris commented Aug 10, 2024

It this the last PR that needs to be merged for a 2.1.0 release candidate?

Not for the rc, and maybe not for the final either if it isn't resolved.

Don't mistake public/private declarations of F90 subroutines for variables
when the corresponding subroutines are filtered by use of only:.
Also, handle modules with no public variables or subroutines, caused by
the filtering.

Closes numpygh-26920.
@m-weigand
Copy link
Contributor Author

Sorry for the late reply, summer vacation got in the way.

I modified the pull request to include:

  • an addition example with no public entities in both modules
  • I moved the check in f90mod_rules.py into auxfuncs.py

Please let me know if I should change anything else.

@charris charris modified the milestones: 2.1.0 release, 2.1.1 release Aug 19, 2024
@mattip mattip requested a review from HaoZeke August 26, 2024 14:08
@HaoZeke
Copy link
Member

HaoZeke commented Aug 27, 2024

LGTM, in it goes, thanks for working on this @m-weigand !

@HaoZeke HaoZeke merged commit f3d48d0 into numpy:main Aug 27, 2024
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 28, 2024
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.

BUG: f2py: Exception when trying to parse private/public subroutine declaration in combination with only: option

4 participants