-
-
Notifications
You must be signed in to change notification settings - Fork 24
Implement GlyphItemIter manually #161
base: master
Are you sure you want to change the base?
Changes from 2 commits
3a70ef0
21ca84e
73f8bf2
aff3ba2
d1cb61c
b2fb7d6
8cbf41c
c2cfba3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
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>); | ||
|
||
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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type not exported, I can change to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I missed that. Better make it more explicit then, yes :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe there should also be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this need be other type instead of this, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because reverse variant (from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also first call There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe all these should actually be |
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be indeed negative? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://gitlab.gnome.org/GNOME/pango/blob/050ebae3a0340e37187319273b5ab443cbe1f0fd/pango/pango-glyph-item.c#L481 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be on RTL text, and I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For extra fun you can make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will try that too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, for this I need write two more iterator impls ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, this function and next unneeded There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated, also removed old unused getters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue that it's a good idea to mark it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it even make a difference if it's not inlined? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some std library API called There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.