Implement GlyphItemIter manually #161

EPashkin · 2019-09-26T10:50:15Z

Fix #160

@velyan Can you confirm that it works?

Note there also two fields in iterator but its seems not changed on iterating and filled only on initialization.

struct PangoGlyphItemIter {
  PangoGlyphItem *glyph_item;
  const gchar *text;

Also text seems ignored, at minimum for Layout its text used anyway,
so I removed parameter from constructor.

cc @GuillaumeGomez, @sdroege

upsuper · 2019-09-27T00:56:05Z

src/glyph_item_iter.rs

+}
+
+impl GlyphItemIter {
+    pub fn init_end(glyph_item: &mut GlyphItem) -> Option<GlyphItemIter> {


I gues this should probably be called new_end or so, as it's creating a new instance of GlyphItemIter rather than just initializing it.

Maybe, I just don't want problem with docs.
@GuillaumeGomez, @sdroege what do you think?

Yes and the signature of the function should look different

@sdroege without mut ?

This does not need a mutable reference is what I meant :)

sdroege · 2019-09-27T07:36:03Z

src/glyph_item_iter.rs

+        }
+    }
+
+    pub fn prev_cluster(&mut self) -> bool {


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

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

Why? This literally is an iterator :)

A lifetime parameter is also needed on this type

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

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

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

sdroege · 2019-09-27T07:38:45Z

src/glyph_item_iter.rs

+                iter.to_glib_none_mut().0,
+                glyph_item.to_glib_none_mut().0,
+                //Text seems ignored and item's used
+                "".to_glib_none().0,


This is wrong. This must be the text corresponding to the glyph item, or otherwise you get invalid memory accesses. No idea how to enforce that :)

See https://gitlab.gnome.org/GNOME/pango/blob/050ebae3a0340e37187319273b5ab443cbe1f0fd/pango/pango-glyph-item.c#L287-288 for where this is used for example.

Strange but for me this works fine for Layout

I don't know, I can only tell you what the code looks like :) It's taking the text pointer without any checks and then adds offsets calculated from the glyph item on it without checking that it's actually in the valid range of the text.

sdroege · 2019-09-27T07:40:08Z

src/glyph_item_iter.rs

+
+glib_wrapper! {
+    #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+    pub struct GlyphItemIter(Boxed<pango_sys::PangoGlyphItemIter>);


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.

sdroege · 2019-09-27T07:41:57Z

src/glyph_item_iter.rs

+        }
+    }
+
+    pub fn start_index(&self) -> i32 {


I believe all these should actually be usize...

EPashkin · 2019-09-27T09:41:26Z

Implement iterator and publish only functions on GlyphItem

…e GlyphItemIter

sdroege · 2019-09-27T09:46:44Z

src/glyph_item_iter.rs

+}
+
+impl GlyphItemIter {
+    pub fn init_end(glyph_item: &GlyphItem, text: &str) -> Option<GlyphItemIter> {


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.

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

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.

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

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

sdroege · 2019-09-27T09:47:03Z

src/glyph_item_iter.rs

+}
+
+impl<'a> GlyphItemIterator<'a> {
+    #[inline(always)]


Hm, this function and next unneeded

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

Updated, also removed old unused getters

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.

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

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.

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.

sdroege · 2019-09-27T09:47:28Z

src/glyph_item_iter.rs

+        GlyphItemIterator::new_start(self, text)
+    }
+
+    pub fn riter<'a>(


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

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

Sounds better to me

sdroege · 2019-09-27T09:48:13Z

src/glyph_item_iter.rs

+    pub end_char: usize,
+}
+
+pub struct GlyphItemIterator<'a> {


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.

I will try that too

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

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

EPashkin · 2019-09-27T10:39:03Z

Changed constructor visiblity and safety,
renamed to reverse_iter

EPashkin · 2019-09-27T11:00:50Z

Replaced is_reverse with type parameter

sdroege · 2019-09-27T11:36:46Z

src/glyph_item_iter.rs

+}
+
+pub struct GlyphItemIteratorData {
+    pub start_glyph: i32,


Can this be indeed negative?

https://gitlab.gnome.org/GNOME/pango/blob/050ebae3a0340e37187319273b5ab443cbe1f0fd/pango/pango-glyph-item.c#L481
but on my tests its minimum is 0

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.

Ok, I change it too

sdroege · 2019-09-27T11:38:07Z

src/glyph_item_iter.rs

+
+    pub fn reverse_iter<'a>(
+        &'a self,
+        text: &'a str,


How do we ensure that the text corresponds to the glyph item? From reading the code it seems like we otherwise would get invalid memory accesses (if the text is smaller than the glyph)

I don't know way to ensure :(

We can get PangoItem.length and compare with text length and chars count, but it not give us 100%

I don't know either, needs someone to dig into this API

@EPashkin This one is the remaining issue? I guess this needs some talking to Pango developers to figure out what to do about this here :)

Any news on this?

@GuillaumeGomez Sorry, but I still have no idea how do this safe,
so better just leave it for now.

velyan · 2019-10-01T05:19:17Z

@EPashkin it works!
Thanks a lot for the speedy fix.

EPashkin · 2019-10-01T05:21:08Z

@velyan Only one problem still unsolved #161 (review) 😢

velyan · 2019-10-01T05:39:56Z

@velyan Only one problem still unsolved #161 (review) 😢

I see, sorry I can't help with this either :/

Implement GlyphItemIter manually

3a70ef0

upsuper reviewed Sep 27, 2019

View reviewed changes

sdroege reviewed Sep 27, 2019

View reviewed changes

src/glyph_item_iter.rs Outdated

}

}

pub fn start_index(&self) -> i32 {

Copy link

Member

sdroege Sep 27, 2019

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

Implement functions, returning iterator, for GlyphItem instead of typ…

21ca84e

…e GlyphItemIter

EPashkin force-pushed the GlyphItemIter branch from 7ee9698 to 21ca84e Compare September 27, 2019 09:46

sdroege reviewed Sep 27, 2019

View reviewed changes

EPashkin added 3 commits September 27, 2019 12:57

Remove unneeded functions

73f8bf2

Mark constructor unsafe and pub(crate)

aff3ba2

Rename riter to reverse_iter

d1cb61c

EPashkin added 2 commits September 27, 2019 13:58

Change iterator mode to type parameter

b2fb7d6

Rename 'into_data` to 'to_data'

8cbf41c

sdroege reviewed Sep 27, 2019

View reviewed changes

Change type of xxx_glyph fields to usize

c2cfba3

Implement GlyphItemIter manually #161

Are you sure you want to change the base?

Implement GlyphItemIter manually #161

Conversation

EPashkin commented Sep 26, 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

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

EPashkin commented Sep 27, 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

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

EPashkin commented Sep 27, 2019

EPashkin commented Sep 27, 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

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

velyan commented Oct 1, 2019

EPashkin commented Oct 1, 2019

velyan commented Oct 1, 2019