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
release: Add sanity checks at beginning of each function #464
Conversation
7774d12
to
bf2b112
Compare
| @@ -273,6 +285,9 @@ as_release_set_version (AsRelease *release, const gchar *version) | |||
| gint | |||
| as_release_vercmp (AsRelease *rel1, AsRelease *rel2) | |||
| { | |||
| g_return_val_if_fail (AS_IS_RELEASE (rel1), 0); | |||
| g_return_val_if_fail (AS_IS_RELEASE (rel2), 0); | |||
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.
Those two checks aren't needed, as as_release_get_version already verifies the input.
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.
It still helps the consumer to know that it is the content that they passed to vercmp that is wrong and also to know if it is rel1 or rel2
|
Thanks! That must have been quite a bit of work to add these - some newer public API has these checks, while they are generally omitted from private and internal API (with some exceptions) as well as older public API (the latter because of laziness, old conventions, and the thought that the checks won't be needed). |
bf2b112
to
284950d
Compare
Allows to fail with a loud message on what happened before an actual issue.
284950d
to
84f106c
Compare
|
For context, we had a crash in AppCenter on our launch day elementary/appcenter#1993 which could have just been a warning with this MR |
I assumed there was a story like this behind this PR :-D |
Allows to fail with a loud message on what happened before an actual issue.