change type of dc_msg_t.text to String #283

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

src/dc_chat.rs

@@ -1001,7 +991,7 @@ unsafe fn set_draft_raw(context: &Context, chat_id: uint32_t, msg: *mut dc_msg_t
    // save new draft
    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?

src/dc_mimefactory.rs

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

src/dc_msg.rs

@@ -827,13 +834,21 @@ pub unsafe fn dc_msg_get_summarytext(
        return dc_strdup(0 as *const libc::c_char);
    }

-    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

src/dc_msg.rs

@@ -1049,8 +1064,7 @@ pub unsafe fn dc_msg_set_text(mut msg: *mut dc_msg_t, text: *const libc::c_char)
    if msg.is_null() || (*msg).magic != 0x11561156i32 as libc::c_uint {
        return;
    }
-    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

src/dc_chat.rs

-            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

src/dc_chat.rs

    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

src/dc_chat.rs

@@ -1001,7 +991,7 @@ unsafe fn set_draft_raw(context: &Context, chat_id: uint32_t, msg: *mut dc_msg_t
    // save new draft
    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

src/dc_chat.rs

@@ -725,7 +725,7 @@ unsafe fn prepare_msg_raw(
                            timestamp,
                            (*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

src/dc_msg.rs

-
-    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.

src/dc_msg.rs

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

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

src/dc_chat.rs

-                    } 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

src/dc_mimefactory.rs

+            .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

src/dc_msg.rs

@@ -31,7 +32,7 @@ pub struct dc_msg_t<'a> {
    pub timestamp_sort: i64,
    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

src/dc_msg.rs

+            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.

@KAction cast 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 35c2daa Compare August 3, 2019 17:07

link2xt reviewed Aug 3, 2019

View reviewed changes

KAction force-pushed the msg-text branch from 35c2daa to dbd337a Compare 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 String Aug 3, 2019

KAction force-pushed the msg-text branch 3 times, most recently from 2054cd0 to fb1622a Compare 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 String Aug 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 0000f0d Compare 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 ded8c12 Compare 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 bde84c6 Compare 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 9ab922a Compare August 4, 2019 18:24

dignifiedquire requested changes Aug 4, 2019

View reviewed changes

src/dc_msg.rs Outdated Show resolved Hide resolved

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 b0d7dad Compare 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 deltachat:master Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change type of dc_msg_t.text to String #283

change type of dc_msg_t.text to String #283

KAction commented Aug 3, 2019

link2xt Aug 3, 2019

link2xt Aug 4, 2019

link2xt Aug 3, 2019

link2xt Aug 3, 2019

link2xt Aug 3, 2019

link2xt commented Aug 3, 2019

KAction commented Aug 3, 2019

KAction commented Aug 4, 2019 •

edited

Loading

KAction commented Aug 4, 2019

link2xt Aug 4, 2019

KAction Aug 4, 2019 •

edited

Loading

link2xt Aug 4, 2019

link2xt Aug 4, 2019

KAction Aug 4, 2019

KAction Aug 4, 2019

link2xt Aug 4, 2019

KAction Aug 4, 2019

link2xt Aug 4, 2019

flub Aug 6, 2019

flub Aug 6, 2019

flub Aug 6, 2019

flub Aug 6, 2019

KAction Aug 6, 2019

KAction Aug 6, 2019

link2xt Aug 7, 2019

KAction commented Aug 7, 2019

link2xt commented Aug 7, 2019

flub commented Aug 7, 2019

change type of dc_msg_t.text to String #283

change type of dc_msg_t.text to String #283

Conversation

KAction commented Aug 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

link2xt commented Aug 3, 2019

KAction commented Aug 3, 2019

KAction commented Aug 4, 2019 • edited Loading

KAction commented Aug 4, 2019

Choose a reason for hiding this comment

KAction Aug 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KAction commented Aug 7, 2019

link2xt commented Aug 7, 2019

flub commented Aug 7, 2019

KAction commented Aug 4, 2019 •

edited

Loading

KAction Aug 4, 2019 •

edited

Loading