-
Notifications
You must be signed in to change notification settings - Fork 1k
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 EXIT_ codes instead of magic numbers for exit(...) and main return values #1609
Comments
Concept ACK I had the same thought in the past. My suggestion to remove |
While working on this I noticed a mistake(?) in the comments in the verification section of ecdsa.c line 95 and 101 (probably other examples too). The comment says "This will return 0 if the signature can't be parsed correctly" however the return value in case of failure is 1. Shall I just adjust it together with the other return values and replace it with EXIT_FAILURE? |
I think what is meant here is that But yeah, the fact that you misread this is proof that this is misleading and should be improved.
That sounds reasonable, yes. At the risk of stating the obvious, it makes sense to have multiple independent but related commits under the same theme in a single PR, at least if they're small. And in this case, the overarching theme of the PR would be "Clean up examples and make them clearer". An exception can be warranted if some of the commits are more important/more urgent, and we want to make sure that these get in and are not held up by lack of review or required changes in the less important commits. |
This is really only a minor issue, but I noticed while reviewing #1479 that the return codes of functions in the examples could potentially be confusing. Throughout the API and internal functions we use 0=failure/1=success, while for the main function and (
exit(...)
) it's the other way round, i.e. 0=success/1=failure. We could useEXIT_{SUCCESS,FAILURE}
(defined in stdlib.h, see https://en.cppreference.com/w/c/program/EXIT_status) for the latter instead for more clarity.See e.g. bitcoin/bitcoin@4441018 for a comparable change in Bitcoin Core as orientation. This could be a good first issue.
The text was updated successfully, but these errors were encountered: