Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Implement GlyphItemIter manually #161

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Gir.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ generate = [
"Pango.FontsetSimple",
"Pango.Glyph",
"Pango.GlyphItem",
"Pango.GlyphItemIter",
"Pango.GlyphString",
"Pango.GlyphUnit",
"Pango.Gravity",
Expand Down Expand Up @@ -209,6 +208,11 @@ status = "generate"
name = "language"
const = true

[[object]]
name = "Pango.GlyphItemIter"
#need manual iterator implementation
status = "ignored"

[[object]]
name = "Pango.Layout"
status = "generate"
Expand Down
56 changes: 0 additions & 56 deletions src/auto/glyph_item_iter.rs

This file was deleted.

3 changes: 0 additions & 3 deletions src/auto/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,6 @@ pub use self::font_metrics::FontMetrics;
mod glyph_item;
pub use self::glyph_item::GlyphItem;

mod glyph_item_iter;
pub use self::glyph_item_iter::GlyphItemIter;

mod glyph_string;
pub use self::glyph_string::GlyphString;

Expand Down
229 changes: 229 additions & 0 deletions src/glyph_item_iter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
// Copyright 2019, The Gtk-rs Project Developers.
// See the COPYRIGHT file at the top-level directory of this distribution.
// Licensed under the MIT license, see the LICENSE file or <http://opensource.org/licenses/MIT>

use glib::translate::*;
use pango_sys;
use GlyphItem;

//Note: This type not exported
glib_wrapper! {
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct GlyphItemIter(Boxed<pango_sys::PangoGlyphItemIter>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a lifetime parameter. The the glyph item used for initialization must stay alive at least as long as the iterator. The iterator "borrows" the item.


match fn {
copy => |ptr| pango_sys::pango_glyph_item_iter_copy(mut_override(ptr)),
free => |ptr| pango_sys::pango_glyph_item_iter_free(ptr),
init => |_ptr| (),
clear => |_ptr| (),
get_type => || pango_sys::pango_glyph_item_iter_get_type(),
}
}

impl GlyphItemIter {
pub fn init_end(glyph_item: &GlyphItem, text: &str) -> Option<GlyphItemIter> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still unsafe: you need to ensure that the parameters stay alive as long as the return iter. And you need to ensure that the text matches the glyph item, otherwise invalid memory accesses will happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type not exported, I can change to pub(crate) to be more explicit,
and only used within lifetimed GlyphItemIterator

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, if soundness of a function depends on its usage (i.e. it doesn't guarantee), it should be marked unsafe regardless of whether it's internal or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I missed that. Better make it more explicit then, yes :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, if soundness of a function depends on its usage (i.e. it doesn't guarantee), it should be marked unsafe regardless of whether it's internal or not.

Yes, definitely. but the function is also gone now, so... :)

unsafe {
let mut iter = GlyphItemIter::uninitialized();
let ret = from_glib(pango_sys::pango_glyph_item_iter_init_end(
iter.to_glib_none_mut().0,
mut_override(glyph_item.to_glib_none().0),
text.to_glib_none().0,
));
if ret {
Some(iter)
} else {
None
}
}
}

pub fn init_start(glyph_item: &GlyphItem, text: &str) -> Option<GlyphItemIter> {
unsafe {
let mut iter = GlyphItemIter::uninitialized();
let ret = from_glib(pango_sys::pango_glyph_item_iter_init_start(
iter.to_glib_none_mut().0,
mut_override(glyph_item.to_glib_none().0),
text.to_glib_none().0,
));
if ret {
Some(iter)
} else {
None
}
}
}

pub fn next_cluster(&mut self) -> bool {
unsafe {
from_glib(pango_sys::pango_glyph_item_iter_next_cluster(
self.to_glib_none_mut().0,
))
}
}

pub fn prev_cluster(&mut self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there should also be Iterator, DoubleEndedIterator impls for easier usage of this, that have a struct IterItem { start_index: i32, ...} as item?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this need be other type instead of this,
with lifetime etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This literally is an iterator :)

A lifetime parameter is also needed on this type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because reverse variant (from iter_end) next() need call prev_cluster

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also first call next need return "current" value instead moving ( for reverse next_back need return that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so might indeed need an adapter struct :) Weird API!

unsafe {
from_glib(pango_sys::pango_glyph_item_iter_prev_cluster(
self.to_glib_none_mut().0,
))
}
}

pub fn start_index(&self) -> i32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all these should actually be usize...

self.0.start_index
}

pub fn start_char(&self) -> i32 {
self.0.start_char
}

pub fn start_glyph(&self) -> i32 {
self.0.start_glyph
}

pub fn end_index(&self) -> i32 {
self.0.end_index
}

pub fn end_char(&self) -> i32 {
self.0.end_char
}

pub fn end_glyph(&self) -> i32 {
self.0.end_glyph
}

pub fn into_data(&self) -> GlyphItemIteratorData {
GlyphItemIteratorData {
start_glyph: self.0.start_glyph,
end_glyph: self.0.end_glyph,
start_index: self.0.start_index as usize,
end_index: self.0.end_index as usize,
start_char: self.0.start_char as usize,
end_char: self.0.end_char as usize,
}
}
}

pub struct GlyphItemIteratorData {
pub start_glyph: i32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be indeed negative?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be on RTL text, and I think the pango_glyph_item_iter_prev_cluster() call before returning would then set that to a positive index again. I don't think this can be negative ever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I change it too

pub start_index: usize,
pub start_char: usize,

pub end_glyph: i32,
pub end_index: usize,
pub end_char: usize,
}

pub struct GlyphItemIterator<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extra fun you can make the is_reverse bool a type parameter for the struct and then get rid of the if in next() and let it all be decided at compile-time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try that too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, for this I need write two more iterator impls ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but each would be half as big. No if/else

item: &'a GlyphItem,
text: &'a str,
is_reverse: bool,
iter: Option<GlyphItemIter>,
}

impl<'a> GlyphItemIterator<'a> {
#[inline(always)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this function and next unneeded

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the inline annotation. Don't add those unless you benchmarked that it actually makes a positive difference :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, also removed old unused getters

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that it's a good idea to mark it #[inline], because otherwise functions without type parameter like this would not be inlined downstream unless the final binary is built with LTO enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it even make a difference if it's not inlined?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have an extra function call whenever downstream invokes it, and if that happens to be in a tight loop, it could be a problem.

My thought is basically that marking it inline wouldn't hurt anything as it's so small. It would just ensure the compiler is able to inline it if it thinks fit.

But maybe applications serious on performance should really enable LTO, so it may not really matter that much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory I agree with you but in practice it's all not that easy and generally a better idea to not do anything unless you actually measure it. The practical effects are not always that obvious. Just like with all optimizations: measure, then fix if needed.

fn new_start(item: &'a GlyphItem, text: &'a str) -> GlyphItemIterator<'a> {
GlyphItemIterator {
item,
text,
is_reverse: false,
iter: None,
}
}

#[inline(always)]
fn new_end(item: &'a GlyphItem, text: &'a str) -> GlyphItemIterator<'a> {
GlyphItemIterator {
item,
text,
is_reverse: true,
iter: None,
}
}
}

impl GlyphItem {
pub fn iter<'a>(
&'a self,
text: &'a str,
) -> impl DoubleEndedIterator<Item = GlyphItemIteratorData> + 'a {
GlyphItemIterator::new_start(self, text)
}

pub fn riter<'a>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some std library API called riter() or what names do they use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it exists, but now I not found any, can be any other name then like reverse_iter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds better to me

&'a self,
text: &'a str,
) -> impl DoubleEndedIterator<Item = GlyphItemIteratorData> + 'a {
GlyphItemIterator::new_end(self, text)
}
}

impl<'a> Iterator for GlyphItemIterator<'a> {
type Item = GlyphItemIteratorData;

fn next(&mut self) -> Option<Self::Item> {
if let Some(ref mut iter) = self.iter {
if self.is_reverse {
if iter.prev_cluster() {
Some(iter.into_data())
} else {
None
}
} else {
if iter.next_cluster() {
Some(iter.into_data())
} else {
None
}
}
} else {
let iter = if self.is_reverse {
GlyphItemIter::init_end(self.item, self.text)
} else {
GlyphItemIter::init_start(self.item, self.text)
};
if let Some(iter) = iter {
let data = iter.into_data();
self.iter = Some(iter);
Some(data)
} else {
None
}
}
}
}

impl<'a> DoubleEndedIterator for GlyphItemIterator<'a> {
fn next_back(&mut self) -> Option<Self::Item> {
if let Some(ref mut iter) = self.iter {
if self.is_reverse {
if iter.next_cluster() {
Some(iter.into_data())
} else {
None
}
} else {
if iter.prev_cluster() {
Some(iter.into_data())
} else {
None
}
}
} else {
let iter = if self.is_reverse {
GlyphItemIter::init_start(self.item, self.text)
} else {
GlyphItemIter::init_end(self.item, self.text)
};
if let Some(iter) = iter {
let data = iter.into_data();
self.iter = Some(iter);
Some(data)
} else {
None
}
}
}
}
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub mod attr_list;
pub mod attribute;
pub mod font_description;
mod functions;
mod glyph_item_iter;
pub mod gravity;
pub mod item;
pub mod language;
Expand Down