Skip to content
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

Don't implement Copy on certain BoxedInline structs #1532

Open
SeaDve opened this issue Nov 1, 2023 · 5 comments
Open

Don't implement Copy on certain BoxedInline structs #1532

SeaDve opened this issue Nov 1, 2023 · 5 comments
Milestone

Comments

@SeaDve
Copy link
Contributor

SeaDve commented Nov 1, 2023

Currently, it implements Copy, which could cause unexpected behaviors. I think it is better to just implement Clone, so the user can explicitly create a copy of the iterator.

This is also the reason why Range from std lib doesn't implement Copy, even though the underlying type implements copy.

Example:

let mut text_start = text_end;
text_start.backward_line();

Does text_end also move backward a line? (Spoiler: it does not, since it is creating a copy behind the scenes)

let mut text_start = text_end.clone();
text_start.backward_line();

Here, it is explicit that we are making a deep copy of the text_end.

@sdroege
Copy link
Member

sdroege commented Nov 1, 2023

That makes sense, yes. We should check other types that currently implement Copy too, here and in gtk-rs-core.

Can you create an MR and go over these?

@bilelmoussaoui bilelmoussaoui added this to the 0.8.0 milestone Nov 1, 2023
@SeaDve
Copy link
Contributor Author

SeaDve commented Nov 2, 2023

A quick look on gtk4-rs which has possibly problematic Copy are the following:

  • GtkTreeIter (Though doesn't seem to have any methods nor impls)
  • GtkTextIter
  • GskRoundedRect (has &mut methods)
  • GtkBorder (has &mut methods)
  • GtkTopLevelSize (has &mut methods)

In gtk-rs-core, there are no iterators, but there are these:

  • PangoGlyphGeometry (has &mut methods)
  • PangoGlyphInfo (has &mut methods)
  • GrapheneMatrix (has &mut methods)
  • GDate (has &mut methods)
  • GrapheneRect (has &mut methods)
  • PangoRect (has &mut methods)
  • PangoMatrix (has &mut methods)
  • GStringBuilder (BoxedInline, but somehow no Copy impl?)

@SeaDve SeaDve changed the title Don't implement Copy on GtkTextIter Don't implement Copy on certain BoxedInline structs Nov 2, 2023
@sdroege
Copy link
Member

sdroege commented Nov 2, 2023

GStringBuilder (BoxedInline, but somehow no Copy impl?)

Only ones that have no copy/clear function are Copy IIRC. If you need to free memory or do more things than a memcpy then it's not Copy and can't be.

The ones you listed seem all OK (gtk-rs-core). For gtk4-rs I'll let @bilelmoussaoui comment.

@bilelmoussaoui
Copy link
Member

GskRoundedRect (has &mut methods)

Not sure how to handle this one

GtkBorder (has &mut methods)

But the type also provides a copy function? gtk_border_copy, where that is supposed to be used in this case?

GtkTopLevelSize (has &mut methods)

This was fixed already

@bilelmoussaoui
Copy link
Member

For TextIter, see also #1280

@bilelmoussaoui bilelmoussaoui modified the milestones: 0.8.0, 0.9.0 May 31, 2024
@sdroege sdroege modified the milestones: 0.9.0, 0.10 Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants