change type of dc_msg_t.text to String by KAction · Pull Request #283 · chatmail/core

KAction · 2019-08-03T12:01:27Z

Also, remove `send-garbage' command from REPL, since it is not possible to send
non-utf8 string anymore.

link2xt · 2019-08-03T12:27:09Z

 if !msg.is_null() {
 if (*msg).type_0 == Viewtype::Text {
- if (*msg).text.is_null() || *(*msg).text.offset(0isize) as libc::c_int == 0i32 {
+ if (*msg).text.is_none() {


You change the logic here. Existing code checks for
(*msg).text.is_none() || (*msg).text.unwrap().is_empty()

@KAction If I remember correctly you added the check for empty string but removed it now?

link2xt · 2019-08-03T12:46:47Z

 }

- dc_msg_get_summarytext_by_raw(
+ let msgtext_ptr = if let Some(ref text) = (*msg).text {


Use map_or here.

link2xt · 2019-08-03T12:47:02Z

 }
- free((*msg).text as *mut libc::c_void);
- (*msg).text = dc_strdup(text);
+ (*msg).text = if text.is_null() { None } else { Some(to_string(text)) };


link2xt · 2019-08-03T17:20:43Z

- if (*msg).text.is_null() || *(*msg).text.offset(0isize) as libc::c_int == 0i32 {
- OK_TO_CONTINUE = false;
+ OK_TO_CONTINUE = match (*msg).text {
+ None => false,


Maybe use .map_or here as well?

link2xt · 2019-08-03T17:26:25Z

I am ok with merging as-is after cargo fmt.

All those map_or suggestions are a matter of taste.

KAction · 2019-08-03T18:49:30Z

Okay. Plus-control passed. Master builds fine.

KAction · 2019-08-04T08:55:19Z

I am ok with merging as-is after cargo fmt.
All those map_or suggestions are a matter of taste.

@link2xt No. Somewhy it breaks tests. Investigating.

KAction · 2019-08-04T15:48:58Z

I think it is ready. The catch was that Option<String> was passed to params!, and, I believe, translated to SQL NULL. It should have caused error somewhere, I think.

link2xt · 2019-08-04T16:30:06Z

 }
+ dbg!(OK_TO_CONTINUE);
 if OK_TO_CONTINUE {
+ dbg!();


Are these dbg!s going to be useful later?

Oops. This is bug. Just fixed.

link2xt · 2019-08-04T16:58:30Z

 if !msg.is_null() {
 if (*msg).type_0 == Viewtype::Text {
- if (*msg).text.is_null() || *(*msg).text.offset(0isize) as libc::c_int == 0i32 {
+ if (*msg).text.is_none() {


@KAction If I remember correctly you added the check for empty string but removed it now?

Previous version of patch, that changed type of `dc_msg_t.text' to String had been breaking `prepare_and_send' test in Python. This commit adds same regression test, reimplemented in Rust, since running Rust tests is simplier and faster.

link2xt · 2019-08-04T17:12:03Z

 (*msg).type_0,
 (*msg).state,
-if !(*msg).text.is_null() { Some(as_str((*msg).text)) } else { None },
+ (*msg).text.deref().unwrap_or(""),


@dignifiedquire previously said that using unstable features is discouraged because the goal is to move to stable Rust. This seems to be the line that requires inner_deref.

KAction · 2019-08-04T17:28:23Z

-
- let res = dc_truncate_str(as_str((*msg).text), 30000);
- res.strdup()
+ if let Some(ref text) = (*msg).text {


Previously, code panic!'ed if msg.text is null. Now, this code returns NULL. Probably this is right thing to do.
On other hand, I think that check for msg.is_null() is wrong. Function should not silently fail on bogus input, it should panic, imho. I spent some time debugging, while dc_get_msg_id(NULL) == 0.

All those is_null checks are from the legacy C code. It accepted nulls everywhere and proceeded as if nothing happened instead of failing. That is in spirit of C, where free accepts NULL.
For Rust code, I am in favor of fail-fast design. Users of the library should avoid passing nulls where data is expected.

Quite off-topic, but I disagree that it is in spirit of C. free is exception, not rule. Most C API cause UB (e.g SIGSEGV) on NULL pointer: fopen, fprint, open, readdir, regcomp, ...
On rust code, good. Will add some asserts.

Will add some asserts.
Not in this PR, though. These changes need to be aligned with bindings/android/desktop/ios code, they may rely on this behavior.

Also, remove `send-garbage' command from REPL, since it is not possible to send non-utf8 string anymore.

flub · 2019-08-06T19:41:12Z

- } else {
- ""
- },
+ (*msg).text.as_ref().map(String::as_str).unwrap_or(""),


.unwrap_or_default() would be the same? Not that I mind.

flub · 2019-08-06T19:48:46Z

+ .text
+ .as_ref()
+ .map(|s| CString::yolo(String::as_str(s)));
+ let msgtext_ptr = msgtext_c.map_or(ptr::null(), |s| s.as_ptr());


Oh, this is nice! I think there are a lot of places that simply do s.as_ptr() instead of the full .map_or(ptr::null(), |s| s.as_ptr()) and that's probably the cause of a bunch of crashes.

flub · 2019-08-06T19:53:09Z

 pub timestamp_sent: i64,
 pub timestamp_rcvd: i64,
- pub text: *mut libc::c_char,
+ pub text: Option<String>,


I'd kind of argue that this should be just be String and use "" if there is nothing. I think the None case is really an artefact of the C API and only needs to be replicated on that boundary layer. A lot of things probably become easier if you can just assume this is always a valid string instead of having to handle the Option.
I don't feel too strongly though, this PR is certainly already an improvement as is!

flub · 2019-08-06T19:58:31Z

+ let contact = dc_create_contact(
+ ctx,
+ b"\x00".as_ptr().cast(),
+ b"dest@example.com\x00".as_ptr().cast(),


You could also write this as "dest@example.com".strdup() I think (and above "".strdup()).
But TIL about .cast() though, nice.

strdup performs allocation, and must be freed manually. I'd rather have Rust do it for me.

PS. Since there is goal to drop unstable feature, cast will be gone eventually. Sad, I liked him.

@KActioncast is stable now: rust-lang/rust#62713

KAction · 2019-08-07T12:49:36Z

Okay. What is missing for merging?

link2xt · 2019-08-07T13:38:18Z

@dignifiedquire has to change review to "approve"

flub · 2019-08-07T18:30:34Z

Let's assume @dignifiedquire is busy and merge anyway. Things can always be changed later.

link2xt approved these changes Aug 3, 2019
View reviewed changes

KAction force-pushed the msg-text branch 2 times, most recently from 6dcc8d8 to 35c2daaCompare August 3, 2019 17:07

link2xt reviewed Aug 3, 2019
View reviewed changes

KAction force-pushed the msg-text branch from 35c2daa to dbd337aCompare August 3, 2019 17:30

KAction changed the title ~~Change type of dc_msg_t.text to String~~WIP: change type of dc_msg_t.text to StringAug 3, 2019

KAction force-pushed the msg-text branch 3 times, most recently from 2054cd0 to fb1622aCompare August 4, 2019 14:42

KAction changed the title ~~WIP: change type of dc_msg_t.text to String~~change type of dc_msg_t.text to StringAug 4, 2019

link2xt mentioned this pull request Aug 4, 2019
SQLite: add NOT NULL constraints where applicable #290
Closed

link2xt requested changes Aug 4, 2019
View reviewed changes

KAction force-pushed the msg-text branch from fb1622a to 0000f0dCompare August 4, 2019 16:48

link2xt requested changes Aug 4, 2019
View reviewed changes

Add Rust clone of python `prepare_and_send' test
3c00ed7
Previous version of patch, that changed type of `dc_msg_t.text' to String had been breaking `prepare_and_send' test in Python. This commit adds same regression test, reimplemented in Rust, since running Rust tests is simplier and faster.

KAction force-pushed the msg-text branch from 0000f0d to ded8c12Compare August 4, 2019 17:03

link2xt approved these changes Aug 4, 2019
View reviewed changes

link2xt requested changes Aug 4, 2019
View reviewed changes

KAction force-pushed the msg-text branch from ded8c12 to bde84c6Compare August 4, 2019 17:23

KAction commented Aug 4, 2019
View reviewed changes

link2xt approved these changes Aug 4, 2019
View reviewed changes

KAction force-pushed the msg-text branch 2 times, most recently from 07d7d54 to 9ab922aCompare August 4, 2019 18:24

dignifiedquire requested changes Aug 4, 2019
View reviewed changes

Comment threadsrc/dc_msg.rs Outdated

Dmitry Bogatov added 2 commits August 4, 2019 19:31

Change type of dc_msg_t.text to String
e8621c2
Also, remove `send-garbage' command from REPL, since it is not possible to send non-utf8 string anymore.

Avoid unstable `inner_deref' feature
b0d7dad

KAction force-pushed the msg-text branch from 9ab922a to b0d7dadCompare August 4, 2019 19:32

link2xt requested a review from dignifiedquire August 4, 2019 20:12

flub approved these changes Aug 6, 2019
View reviewed changes

flub mentioned this pull request Aug 6, 2019
Add more assertions to `deltachat-ffi' library #291
Merged

flub merged commit bc23145 into chatmail:masterAug 7, 2019

Uh oh!

Uh oh!

Conversation

KAction commented Aug 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

link2xt commented Aug 3, 2019

Uh oh!

KAction commented Aug 3, 2019

Uh oh!

KAction commented Aug 4, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KAction commented Aug 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KActionAug 4, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KAction commented Aug 7, 2019

Uh oh!

link2xt commented Aug 7, 2019

Uh oh!

flub commented Aug 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KAction commented Aug 4, 2019•
edited
Loading

KActionAug 4, 2019•
edited
Loading