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

Preserve import statement functions - take II #283

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Conversation

awnawab
Copy link
Contributor

@awnawab awnawab commented Apr 11, 2024

A second take on preserving statement function imports via c-style headers. This time, rather than only renaming imports with intfb in their name, we now skip all imports with func in their name. I do realise that this introduces another IFS specific naming convention to the DependencyTransformation, and this probably needs generalising.

Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/283/index.html

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.88%. Comparing base (f4a0c33) to head (1301302).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
+ Coverage   92.82%   92.88%   +0.05%     
==========================================
  Files         102      102              
  Lines       18220    18247      +27     
==========================================
+ Hits        16913    16948      +35     
+ Misses       1307     1299       -8     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 92.85% <100.00%> (+0.06%) ⬆️
transformations 92.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks, as discussed offline this seems like a justifiable variant of baking in IFS-specific knowledge but minimal impact. The blanket check for func in the import name might create problems, though, so I've suggested a slightly different approach.

Comment on lines 302 to 303
target_symbol = im.module.split('.')[0].lower()
if targets and target_symbol.lower() in targets:
if targets and target_symbol.lower() in targets and not 'func' in im.module.lower():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for nitpicking further but just realized that this may fail in pretty trivial cases, e.g., #include "myfunc.intfb.h" will now not be processed... 😅

Should we apply the IFS-specific knowledge a bit more explicitly, e.g. like this?

target_symbol, *suffices = im.module.lower().split('.', maxsplit=1)
if targets and target_symbol in targets and 'func.h' not in suffices:

Note that this works even if no . is in the module name at all, returning suffices as an empty list.
That way this becomes a very loose restriction specific to the problem of statement functions and in-line with the coding standards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah of course, well spotted and apologies for the clumsy oversight 😅

@reuterbal reuterbal requested a review from mlange05 April 11, 2024 08:59
@awnawab awnawab force-pushed the naan-stmt-func-import branch 2 times, most recently from ef023b4 to b6993c7 Compare April 11, 2024 10:02
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the requested changes and bonus points for capturing this edge case in the tests :)

@reuterbal reuterbal added the ready for merge This PR has been approved and is ready to be merged label Apr 11, 2024
Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Looks great and is confirmed clean with ec-physics regression. GTG :shipit:

@reuterbal reuterbal merged commit e1c385a into main Apr 11, 2024
12 checks passed
@reuterbal reuterbal deleted the naan-stmt-func-import branch April 11, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants