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

Add exceptions to precice errors #1594

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from
Draft

Add exceptions to precice errors #1594

wants to merge 12 commits into from

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Mar 6, 2023

Main changes of this PR

This PR

  • adds an exception hierarchy.
  • extends PRECICE_ERROR(precice::APIError, "Oh no!") to take an exception type first.
  • migrates all error calls to exceptions
  • catches exceptions and aborts in the C and Fortran bindings

Motivation and additional information

This allows us to test failing cases.

Closes #280
Closes #1189

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I sticked to C++17 features.
  • I sticked to CMake version 3.16.3.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@fsimonis fsimonis self-assigned this Mar 6, 2023
@fsimonis fsimonis added maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. breaking change Breaks backwards compatibility and users need to act labels Mar 6, 2023
@fsimonis fsimonis added this to the Version 3.0.0 milestone Mar 6, 2023
@davidscn davidscn self-requested a review March 8, 2023 11:51
Copy link
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

Looks good and will make out live easier in the future.

Only a few comments:

  • although it is somewhat of an open question on how we treat the exceptions in C and Fortran, we duplicate quite a lot of code due to the usage of try{} catch (...) {}
  • our test coverage will now complain if we don't test the throw's in our CI
  • we need to export the exceptions header to the (user) installation include target

I would propose to keep the number of exceptions in preCICE rather small (maybe the four most significant) and extend later on demand. We could summarize smaller groups in precice::InternalError or something similar. Let's discuss in person.

Comment on lines +471 to +473
try {
return precice::versionInformation;
} catch (...) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this function throw at all?

Comment on lines +7 to +11
class Error : public std::runtime_error {
public:
Error(const std::string &what_arg)
: std::runtime_error(what_arg){};
};
Copy link
Member

Choose a reason for hiding this comment

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

I agree with the suggestion of @davidscn to keep the number of errors small for the beginning and find out what we need later. The main motivation here is to simplify using the correct error in the correct situation. Two concrete ideas:

  • Only use a single exception for now: Error. We can extend this later? Then there is no risk of wrong or inconsistent usage of error classes, because everything is simple.
  • The errors below are basically one error class per package. From my perspective this means that there is implicitly the rule: "Use ActionError, if you want to throw an error from precice::action". This rule is simple enough to not mess up things from the beginning, but we should also put this into the documentation of each error class.

Comment on lines +13 to +17
class ActionError : public precice::Error {
public:
ActionError(const std::string &what_arg)
: precice::Error(what_arg){};
};
Copy link
Member

Choose a reason for hiding this comment

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

Related to my comment above. I think we should put this error into the namespace precice::action. This makes it clear that the error should only be thrown from this package.

@fsimonis
Copy link
Member Author

fsimonis commented Mar 9, 2023

We decided to:

  • drop MeshError
  • drop M2NError (unused)
  • rename IOError to ExportError
  • Use ExportError for the WatchPoint and WatchIntegral. Move them from precice/impl -> io
  • eventually rename the io package to export

Users should only be interested in catching a precice::Error.
We need other more refined errors in the tests.

@fsimonis fsimonis modified the milestones: Version 3.0.0, Version 3.x.x Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaks backwards compatibility and users need to act maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let preCICE only exit on critical errors Exception Handling of preCICE errors
3 participants