GString refactor by jf2048 · Pull Request #600 · gtk-rs/gtk-rs-core

jf2048 · 2022-03-10T18:01:27Z

Update: See comment #600 (comment) for the general status of this PR.

Original comment: All of these TryFrom are panicking when they don't really need to. This is probably going to break some other things, I'd like to check this against gtk-rs and gstreamer-rs at least. Another option is to leave the panicking From traits in and also support TryFrom.

Also makes GString deref as a GStr

sdroege · 2022-03-11T07:50:08Z

Also needs to be updated with GStr bits if #595 is merged

It's merged :)

jf2048 · 2022-03-11T16:29:07Z

More PRs to address breakage caused by this PR:

Probably they should be merged all at once, or before this one

sdroege · 2022-03-12T08:54:25Z

Next step: change all functions that take &str to instead take impl AsRef<GStr> or so to avoid all the unnecessary memory copies :)

sdroege · 2022-03-12T09:29:56Z


 impl Deref for GString {
-    type Target = str;
+    type Target = GStr;


I guess we should think about the advantages / disadvantages of this. What does a &GStr bring in comparison to just having a &str?

Do we have functions taking GStr only?

Not yet, see my other comments

Maybe better to use str for the time being until we make the switch then.

I'll take this out, but gtk-rs/gtk3-rs#739 and gtk-rs/gir#1327 can still be merged without this if you want. They don't break anything.

Sounds like a good idea regardless, yes. More future-proof.

jf2048 · 2022-03-13T16:03:41Z

Now added impl Default for GString, and an unsafe GString::from_utf8_unchecked if someone wants to skip all the extra checking added by the TryFroms

jf2048 · 2022-03-13T16:26:24Z

Next step: change all functions that take &str to instead take impl AsRef<GStr> or so to avoid all the unnecessary memory copies :)

That was my thought too. We can test it out later with some of the manually implemented functions, and then afterwards work it into gir.

bilelmoussaoui · 2022-03-14T08:33:43Z

While you are updating GString traits, maybe we should have Default impls here. See the gstreamer-rs PR for context

sdroege

Seems OK to me apart from that final comment

sdroege · 2022-03-14T08:36:11Z

While you are updating GString traits, maybe we should have Default impls here. See the gstreamer-rs PR for context

It's here already

sdroege · 2022-11-20T09:43:05Z

Looks generally nice to me. Are you aware of any drawbacks here or does it all seem nice to you?

Also this should probably get a change to gir at some point to

Use impl IntoGStr for transfer none
Use impl Into<GString> for transfer full
parameters.

jf2048 · 2022-12-12T02:13:23Z

Some drawbacks that still persist:

Unfortunate that it is not possible anymore to safely convert from GStr to CStr without re-scanning the string. But I think it is more "Rusty" to make paying that cost optional.
Still have to check all strings coming from C for valid UTF-8. No way to fix that right now as C has no real guarantees about strings.
The previously mentioned issue with needing None::<&str> or GStr::NONE.
There are a ton of GString constructors and error types now, could be confusing. Not sure if all are strictly necessary but in any case it was trivial to implement them in terms of each other.
The Inline variant is limited to 22 chars, it could be optimized further to hold an additional byte with some unsafe hackery to combine the length (5 bits) and the discriminant (2 bits).

jf2048 · 2022-12-13T23:18:19Z

I think those issues are all design issues or can be fixed later, so this is ready now IMO

For consistency with str::as_bytes. Since we always store the length of the slice we can always do a const conversion like str does.

Optimizes GString to avoid unnecessary checking for interior nul bytes. Interior nul bytes will now only be checked upon request. When converting to C strings, the string will always get truncated to the first nul byte. Since GString can now contain interior nul bytes in some circumstances, its interior representation is now Option<Box<str>>. Also adds custom error types for conversions.

For const std::slice::from_raw_parts, stabilized in rust-lang/rust#97522

Also use Cow<[u8]> as storage to optimize out empty string allocations.

add some PartialEq and PartialOrd implementations for GStr too

These traits can accept both &str or &GStr as an argument. A copy is made when passing &str, but avoided when passing &GStr.

jf2048 mentioned this pull request Mar 10, 2022

Structured Logging #595

Merged

jf2048 force-pushed the gstring-traits branch 2 times, most recently from cb4667d to 5cbef4c Compare March 10, 2022 18:19

jf2048 force-pushed the gstring-traits branch 2 times, most recently from 6da3b98 to fe45283 Compare March 11, 2022 13:47

jf2048 changed the title ~~WIP: glib: Use TryFrom for GString conversions~~ WIP: glib: Update GString traits Mar 11, 2022

jf2048 mentioned this pull request Mar 11, 2022

codegen: Use GString::as_str instead of Option<GString>::as_deref gtk-rs/gir#1327

Merged

jf2048 changed the title ~~WIP: glib: Update GString traits~~ glib: Update GString traits Mar 11, 2022

jf2048 marked this pull request as ready for review March 11, 2022 16:30

sdroege requested review from GuillaumeGomez, bilelmoussaoui and sdroege March 12, 2022 08:52

sdroege reviewed Mar 12, 2022

View reviewed changes

Comment thread glib/src/gstring.rs Outdated

jf2048 force-pushed the gstring-traits branch 3 times, most recently from 9e2ca9c to b2d545a Compare March 13, 2022 15:38

jf2048 marked this pull request as draft March 13, 2022 15:41

jf2048 force-pushed the gstring-traits branch from b2d545a to e3253ff Compare March 13, 2022 16:00

jf2048 force-pushed the gstring-traits branch from e3253ff to 294644d Compare March 13, 2022 16:41

jf2048 marked this pull request as ready for review March 13, 2022 18:48

sdroege reviewed Mar 14, 2022

View reviewed changes

Comment thread glib/src/gstring.rs Outdated

sdroege reviewed Mar 14, 2022

View reviewed changes

sdroege reviewed Nov 20, 2022

View reviewed changes

Comment thread glib/src/gstring.rs

jf2048 force-pushed the gstring-traits branch 4 times, most recently from a938c6b to 51f87df Compare November 26, 2022 04:18

bilelmoussaoui reviewed Nov 26, 2022

View reviewed changes

Comment thread glib/src/utils.rs

jf2048 force-pushed the gstring-traits branch 2 times, most recently from bf3d2f1 to f984f68 Compare December 12, 2022 01:24

jf2048 force-pushed the gstring-traits branch from f984f68 to a165b19 Compare December 13, 2022 23:15

jf2048 force-pushed the gstring-traits branch from a165b19 to 8e5ad99 Compare December 13, 2022 23:59

glib: Add #[inline] to more GString trait methods

55e7db2

sdroege force-pushed the gstring-traits branch from 8e5ad99 to f593acb Compare December 24, 2022 21:02

jf2048 added 13 commits December 24, 2022 23:06

glib: implement Display for GStr

9840c3b

glib: rename GStr::to_bytes to as_bytes

02ade78

For consistency with str::as_bytes. Since we always store the length of the slice we can always do a const conversion like str does.

glib: bump required Rust version to 1.64

a577a12

For const std::slice::from_raw_parts, stabilized in rust-lang/rust#97522

glib: make some GStr methods const

0c15b29

glib: have ToGlibPtr check interior string nuls in debug mode

37c7b18

Also use Cow<[u8]> as storage to optimize out empty string allocations.

glib: add more From implementations for GString

5368f27

glib: add ToGlibPtr implementations for more string types

fbc1814

glib: add GStringBuilder::as_gstr and AsRef<GStr>

9753a72

add some PartialEq and PartialOrd implementations for GStr too

glib: add gformat! for directly formatting into a GString

3caed0f

glib: add IntoGStr traits

d4335b5

These traits can accept both &str or &GStr as an argument. A copy is made when passing &str, but avoided when passing &GStr.

glib: allow GString to store small inline strings

c77cbfc

glib: convert some functions to use IntoGStr

8fc9217

sdroege force-pushed the gstring-traits branch from f593acb to 8fc9217 Compare December 24, 2022 21:32

sdroege merged commit 8a9d3be into gtk-rs:master Dec 24, 2022

Uh oh!

Uh oh!

Conversation

jf2048 commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdroege commented Mar 11, 2022

Uh oh!

jf2048 commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdroege commented Mar 12, 2022

Uh oh!

sdroege Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

sdroege Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

jf2048 Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

sdroege Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jf2048 commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jf2048 commented Mar 13, 2022

Uh oh!

Uh oh!

bilelmoussaoui commented Mar 14, 2022

Uh oh!

sdroege left a comment

Choose a reason for hiding this comment

Uh oh!

sdroege commented Mar 14, 2022

Uh oh!

Uh oh!

sdroege commented Nov 20, 2022

Uh oh!

Uh oh!

jf2048 commented Dec 12, 2022

Uh oh!

jf2048 commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jf2048 commented Mar 10, 2022 •

edited

Loading

jf2048 commented Mar 11, 2022 •

edited

Loading

jf2048 commented Mar 13, 2022 •

edited

Loading