-
-
Notifications
You must be signed in to change notification settings - Fork 169
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
try { | ||
return precice::versionInformation; | ||
} catch (...) { |
There was a problem hiding this comment.
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?
class Error : public std::runtime_error { | ||
public: | ||
Error(const std::string &what_arg) | ||
: std::runtime_error(what_arg){}; | ||
}; |
There was a problem hiding this comment.
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.
class ActionError : public precice::Error { | ||
public: | ||
ActionError(const std::string &what_arg) | ||
: precice::Error(what_arg){}; | ||
}; |
There was a problem hiding this comment.
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.
We decided to:
Users should only be interested in catching a precice::Error. |
Main changes of this PR
This PR
PRECICE_ERROR(precice::APIError, "Oh no!")
to take an exception type first.Motivation and additional information
This allows us to test failing cases.
Closes #280
Closes #1189
Author's checklist
pre-commit
hook to prevent dirty commits and usedpre-commit run --all
to format old commits.make changelog
if there are user-observable changes since the last release.Reviewers' checklist