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

Use return() instead of exit() #183

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

havardAasen
Copy link
Contributor

Previously, the function employed exit(), which, when invoked, results in the immediate termination of the entire program. In a library this is not a wanted response when handling errors, a better approach is to return an error code and let the calling application handle the error.

Previously, the function employed `exit()`, which, when invoked, results in the immediate termination of the entire program. In a library this is not a wanted response when handling errors, a better approach is to return an error code and let  the calling application handle the error.
@CLAassistant
Copy link

CLAassistant commented Dec 20, 2023

CLA assistant check
All committers have signed the CLA.

@tkralphs
Copy link
Member

Good catch, this is certainly bad. Unfortunately, the error handling in this file doesn't seem to be too consistent. But this certainly doesn't make things worse.

@tkralphs tkralphs merged commit 477c9cd into coin-or:master Dec 20, 2023
12 of 14 checks passed
@tkralphs
Copy link
Member

For this to make it into release, I'll need to cherry-pick it over into stable/0.108 and then make a release. I think I'll wait on making releases, though, until you are done with your testing/review. Just let me know.

@havardAasen
Copy link
Contributor Author

For this to make it into release, I'll need to cherry-pick it over into stable/0.108 and then make a release. I think I'll wait on making releases, though, until you are done with your testing/review. Just let me know.

Thanks, it's appreciated.

So far I have only found one more issue and that is the same flaw as was backported in CoinUtils, in coin-or/CoinUtils#217.

I looked at the some of the other COIN-OR projects, they all have this same issue.

@tkralphs
Copy link
Member

Hmm, this is a bit mysterious. The patch was apparently in response to this issue: coin-or/CoinUtils#124. However, this patch alone wouldn't have fixed the issue, according to what you're saying. It should have then come up with the unit tests of other projects. Anyway, fixing them all seems like a good idea, but it may take a bit of time if I need to do it myself.

@havardAasen
Copy link
Contributor Author

Hmm, this is a bit mysterious. The patch was apparently in response to this issue: coin-or/CoinUtils#124. However, this patch alone wouldn't have fixed the issue, according to what you're saying. It should have then come up with the unit tests of other projects. Anyway, fixing them all seems like a good idea, but it may take a bit of time if I need to do it myself.

Ok, i might have made it confusing by writing it in this PR. The issue I thought of is not related to this PR at all.

@havardAasen
Copy link
Contributor Author

Sorry @tkralphs I was a bit hasty, forget what I wrote here #183 (comment), I will make a separate issue.

@havardAasen havardAasen deleted the patch-1 branch December 21, 2023 07:14
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.

3 participants