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

refactor(ListModule): trim dependencies, add defaults, improve docstrings #1484

Merged
merged 10 commits into from
Dec 7, 2023

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Dec 6, 2023

  • remove SimModule dependency and sim_message()/stop_with_error() usages
    • minimize dependencies of generic utilities on simulation modules so latter can use former without circularity
    • on programmer error, just print error message and error stop 1
  • use associated(x, y) as default isEqual procedure for ContainsObject
  • more descriptive docstrings, use compact format

@wpbonelli wpbonelli marked this pull request as ready for review December 6, 2023 13:39
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the dependency on stop_with_error(). It is in GenericUtilitiesModule, which does depend on SimVariablesModule, but the dependencies are minimal.

Comment on lines 257 to 258
print *, 'Programming error in ListType%insert_after'
error stop 1
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to check further on the ramifications of this change. I'm thinking stop_with_error() and its use of call exit() was used intentionally, perhaps for API behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think error stop 1 is equivalent to call exit(1) (with possibly wider compiler support), both terminating with the given exit code. On the other hand stop 1 prints the ret code to stderr but exits with code 0, which I imagine is what we want to avoid?

At least that is my reading of the following

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since gnu, intel, and flang all support an exit() intrinsic subroutine maybe it makes sense to standardize on that directly, and stop_with_error() would be unnecessary

One drawback of error stop is it only supports a variable status code in f2018, while exit() already does

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to keep stop_with_error() in place and use it for all programming errors? It could remain the same or we could pass it a message. If we ever needed to change the behavior, we would only need to change it one place? Or we could get rid of it and just call exit().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to keep stop_with_error() in place and use it for all programming errors? It could remain the same or we could pass it a message. If we ever needed to change the behavior, we would only need to change it one place?

That seems good to me. Maybe it could live in a separate module so generic utilities don't depend on simulation-related things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

Comment on lines 274 to 275
print *, "Programming error in ListType%InsertBefore"
error stop 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting this is in here. This must be a remnant from a while back.

@wpbonelli wpbonelli merged commit ae9e0e1 into MODFLOW-ORG:develop Dec 7, 2023
@wpbonelli wpbonelli deleted the listtype branch December 7, 2023 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants