Add more assertions to `deltachat-ffi' library #291
Conversation
KAction
force-pushed
the
ffi-more-asserts
branch
from
f118337 to
7c3fbe0
Compare
KAction
force-pushed
the
ffi-more-asserts
branch
from
7c3fbe0 to
aa1219b
Compare
There was a problem hiding this comment.
While the extra checks are useful invariants to ensure I'm not sure if introducing more panics is generally the right way to go on the C API. Maybe the same invariants could be checked but with a normal error return (including doing so for the existing asserts)? The downside is that there are often few ways to signal what went wrong I guess, but maybe that's just fine if you passed in NULL in the first place.
So I guess this is in line with what the existing rust-based C-API does, but is it the right thing to do for the C API? How did the original API behave? /cc @r10s @dignifiedquire
| let key = config::Config::from_str(dc_tools::as_str(key)).expect("invalid key"); | ||
|
|
||
| // TODO: Translating None to NULL would be more sensible than translating None | ||
| // to "", as it is now. |
There was a problem hiding this comment.
@r10s can you comment on whether the current behaviour is correct please? Or should it indeed be NULL?
There was a problem hiding this comment.
What is correct? I did not change behavior with my patch, but API that deliberately merges missing key and key with value "" is dubious.
| let or_msg_type2 = | ||
| from_prim(or_msg_type2).expect(&format!("incorrect or_msg_type2 = {}", or_msg_type2)); | ||
| let or_msg_type3 = | ||
| from_prim(or_msg_type3).expect(&format!("incorrect or_msg_type3 = {}", or_msg_type3)); |
There was a problem hiding this comment.
These expect calls turn what used to be handled gracefully into a crash. Is this the desired scenario? Should the C API be this strict?
There was a problem hiding this comment.
Yes, this change is by intention. This expect is about incorrect library usage. Pre-condition violation is exactly what assert for.
|
Oh, saw the conversation in #283 so I'm fine with this. Please change the constants to use the names though. |
Change code to panic! on invalid input (null pointers, out-of-range identifiers) instead of silently doing nothing.
KAction
force-pushed
the
ffi-more-asserts
branch
from
aa1219b to
67176c7
Compare
Change code to panic! on invalid input (null pointers, out-of-range
identifiers) instead of silently doing nothing.