Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> So... can this go in? Since there is a suspicion that the code is going to be slow, it would be better to write it to be faster, like (untested): + ParagraphList::const_iterator it = pars_.begin(); + ParagraphList::const_iterator const end = pars_.end(); + while (it != end && &(*it) != &par) + ++it; + + // Realize against environment font information + if (iit != end) + font.realize(outerFont(pit, pars_)); + Of course, profiling would be interesting too :) JMarc
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Thu, Nov 10, 2005 at 04:22:47PM +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > Martin> So... can this go in? > > Since there is a suspicion that the code is going to be slow, it would > be better to write it to be faster, like (untested): > > + ParagraphList::const_iterator it = pars_.begin(); > + ParagraphList::const_iterator const end = pars_.end(); > + while (it != end && &(*it) != &par) > + ++it; > + > + // Realize against environment font information > + if (iit != end) > + font.realize(outerFont(pit, pars_)); > + > > Of course, profiling would be interesting too :) Why would that be faster (I'm happy to comply, but why?)? BTW I loaded the user guide, and noticed no slow down even towards the end when going into lists envs. - Martin pgpG6owg7JNGD.pgp Description: PGP signature
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> Why would that be faster (I'm happy to comply, but why?)? do not recompute pars_size() repeatedly do not invoke operator[] repeatedly, it may be expensive. All this things are small, but there is not much more in the loop, so everything is important. I have a different idea right now: since ParagraphList is a vector, can't we just say that pit = &par - &pars_[0]; ? JMarc
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Jean-Marc Lasgouttes wrote: > I have a different idea right now: since ParagraphList is a vector, > can't we just say that > pit = &par - &pars_[0]; > ? I'm not saying that would be a bad thing to do, but would note that the fact that ParagraphList is a std::vector is meant to be an implementation detail... -- Angus
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
> "Angus" == Angus Leeming <[EMAIL PROTECTED]> writes: Angus> Jean-Marc Lasgouttes wrote: >> I have a different idea right now: since ParagraphList is a vector, >> can't we just say that pit = &par - &pars_[0]; ? Angus> I'm not saying that would be a bad thing to do, but would note Angus> that the fact that ParagraphList is a std::vector is meant to Angus> be an implementation detail... I thought about that :) So, I guess boost has an answer for us, or boost would not be boost... JMarc
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Thu, Nov 10, 2005 at 04:11:03PM +, Angus Leeming wrote: > Jean-Marc Lasgouttes wrote: > > I have a different idea right now: since ParagraphList is a vector, > > can't we just say that > > pit = &par - &pars_[0]; > > ? > > I'm not saying that would be a bad thing to do, but would note that > the fact that ParagraphList is a std::vector is meant to be an > implementation detail... Not so that you'd notice if you browse the code base... Anyway this is a great idea at this point. The cost is a small constant instead of linear with doc size. Tested and works. OK to go in? - Martin (beating his forehead, 'of course!') pgpYCLEc9waa6.pgp Description: PGP signature
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> Anyway this is a great idea at this point. The cost is a small Martin> constant instead of linear with doc size. Martin> Tested and works. OK to go in? I'd rather see the patch first. You may want to BOOST_ASSERT(&pars_[pit] == par); and/or BOOST_ASSERT(pit >=0 && pit < pars_.size()); JMarc
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Thu, 2005-11-10 at 18:08 +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > Martin> Anyway this is a great idea at this point. The cost is a small > Martin> constant instead of linear with doc size. > > Martin> Tested and works. OK to go in? > > I'd rather see the patch first. > > You may want to > BOOST_ASSERT(&pars_[pit] == par); > and/or > BOOST_ASSERT(pit >=0 && pit < pars_.size()); Why not both. Good idea, and still works :-) Actually by now this is more your patch than mine. Why don't you commit it? - Martin Index: text2.C === RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/text2.C,v retrieving revision 1.634 diff -u -p -r1.634 text2.C --- text2.C 25 Oct 2005 09:14:11 - 1.634 +++ text2.C 10 Nov 2005 17:35:09 - @@ -190,6 +190,13 @@ LyXFont LyXText::getFont(Paragraph const if (!isMainText()) applyOuterFont(font); + pit_type pit = &par - &pars_[0]; + BOOST_ASSERT(&pars_[pit] == &par); + BOOST_ASSERT(pit >=0 && pit < pars_.size()); + // Realize against environment font information + if (pit < pars_.size()) + font.realize(outerFont(pit, pars_)); + // Realize with the fonts of lesser depth. font.realize(defaultfont_); signature.asc Description: This is a digitally signed message part
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> Why not both. Good idea, and still works :-) + BOOST_ASSERT(pit >=0 && pit < pars_.size() && &pars_[pit] == &par); would be even better. And also a comment that this is going to break hard with anything else than vector. Martin> Actually by now this is more your patch than mine. Why don't Martin> you commit it? Because I am leaving for home and won't be able to commit until monday (tomorrow is a holiday in France). JMarc
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Jean-Marc Lasgouttes <[EMAIL PROTECTED]> writes: | > "Angus" == Angus Leeming <[EMAIL PROTECTED]> writes: | | Angus> Jean-Marc Lasgouttes wrote: | >> I have a different idea right now: since ParagraphList is a vector, | >> can't we just say that pit = &par - &pars_[0]; ? | | Angus> I'm not saying that would be a bad thing to do, but would note | Angus> that the fact that ParagraphList is a std::vector is meant to | Angus> be an implementation detail... | | I thought about that :) | | So, I guess boost has an answer for us, or boost would not be boost... find_if with some fancy binding, I'll find it elsewhere in the code... it is not bad to create a helper function even if used only one place... it something tries to find something... call it "find" -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Jean-Marc Lasgouttes <[EMAIL PROTECTED]> writes: | > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: | | Martin> So... can this go in? | | Since there is a suspicion that the code is going to be slow, it would | be better to write it to be faster, like (untested): | | + ParagraphList::const_iterator it = pars_.begin(); | + ParagraphList::const_iterator const end = pars_.end(); | + while (it != end && &(*it) != &par) | + ++it; | + for (; it != end; ++it) if (&(*it) == &par) break; should not generate any worse code.. and follows the for(...) idom. but I would prefere ParagraphList::const_iterator it = find_if(pars_.begin(), pars_.end(), some_functor(par)); | + // Realize against environment font information | + if (iit != end) | + font.realize(outerFont(pit, pars_)); | + if (it != pars_.end()) ...; (or give pars_.end() its own temp var if worried about callind end() twice. | Of course, profiling would be interesting too :) Clearity wins. -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Martin Vermeer <[EMAIL PROTECTED]> writes: | Index: text2.C | === | RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/text2.C,v | retrieving revision 1.634 | diff -u -p -r1.634 text2.C | --- text2.C 25 Oct 2005 09:14:11 - 1.634 | +++ text2.C 10 Nov 2005 17:35:09 - | @@ -190,6 +190,13 @@ LyXFont LyXText::getFont(Paragraph const | if (!isMainText()) | applyOuterFont(font); | | + pit_type pit = &par - &pars_[0]; No... this is in the ball park of pointer arithmetic Hmm... I think I basically dislike all of this... passing pointers... passing offset conversion one way, then the other... -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Fri, Nov 11, 2005 at 12:30:01AM +0100, Lars Gullik Bjønnes wrote: > Martin Vermeer <[EMAIL PROTECTED]> writes: > | + pit_type pit = &par - &pars_[0]; > > > No... this is in the ball park of pointer arithmetic > > Hmm... I think I basically dislike all of this... passing pointers... > passing offset conversion one way, then the other... But it's a possibly significant performance issue... and this code is going to be refactored later anyway. So I propose to put it in for now with a FIXME. But I'm happy with a loop too... as long as the bug gets fixed. Just tell me what to do ;-) - Martin
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Martin Vermeer <[EMAIL PROTECTED]> writes: | On Fri, Nov 11, 2005 at 12:30:01AM +0100, Lars Gullik Bjønnes wrote: | > Martin Vermeer <[EMAIL PROTECTED]> writes: | | | > | + pit_type pit = &par - &pars_[0]; | > | > | > No... this is in the ball park of pointer arithmetic | > | > Hmm... I think I basically dislike all of this... passing pointers... | > passing offset conversion one way, then the other... | | But it's a possibly significant performance issue... and this code is | going to be refactored later anyway. "possibly significant" shoots it down for me... either it is, or it isn't. Unless we have real data, we go for the maintainable sulution. | | So I propose to put it in for now with a FIXME. | | But I'm happy with a loop too... as long as the bug gets fixed. Just | tell me what to do ;-) next mail, find. -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Fri, Nov 11, 2005 at 12:25:50AM +0100, Lars Gullik Bjønnes wrote: > | + ParagraphList::const_iterator it = pars_.begin(); > | + ParagraphList::const_iterator const end = pars_.end(); > | + while (it != end && &(*it) != &par) > | + ++it; > | + > > for (; it != end; ++it) > if (&(*it) == &par) > break; > > should not generate any worse code.. and follows the for(...) idom. > > but I would prefere > > ParagraphList::const_iterator it = > find_if(pars_.begin(), pars_.end(), some_functor(par)); Why? It's obviously more code to type, to read, and not simpler to understand. You obviously could write out the for loop but cut the corner in the some_functor case. [Thing would obviously different with lambda...] > | Of course, profiling would be interesting too :) > > Clearity wins. IMO the for loop is much clearer... Andre'
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Thu, Nov 10, 2005 at 05:41:37PM +0100, Jean-Marc Lasgouttes wrote: > > "Angus" == Angus Leeming <[EMAIL PROTECTED]> writes: > > Angus> Jean-Marc Lasgouttes wrote: > >> I have a different idea right now: since ParagraphList is a vector, > >> can't we just say that pit = &par - &pars_[0]; ? > > Angus> I'm not saying that would be a bad thing to do, but would note > Angus> that the fact that ParagraphList is a std::vector is meant to > Angus> be an implementation detail... > > I thought about that :) A coomment would be enough. > So, I guess boost has an answer for us, or boost would not be boost... std::distance. Andre'
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Thu, Nov 10, 2005 at 03:28:42PM +0200, Martin Vermeer wrote:
> On Wed, 2005-11-09 at 14:17 +0100, Jean-Marc Lasgouttes wrote:
> > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
> >
> > Martin> So then the original patch should go in, with break added? How
> > Martin> expensive is this with big documents? It scales linearly.
> >
> > The good thing is that we have really reduce the number of times
> > getFont gets called. Nevertheless, the maybe quadratic in some cases,
> > since getFont is called at least once per paragraph.
> >
> > The right solution would be to pass a DocIterator or a pit/pos pair to
> > getFont, but this is probably a lot of work.
> >
> > JMarc
>
> So... can this go in?
+ pit_type pit = pars_.size();
+ for (pit_type it = 0; it < pars_.size(); ++it) {
+ if (&pars_[it] == &par) {
+ pit = it;
+ break;
+ }
+ }
I just wonder whether
+ pit_type pit = 0;
+ for ( ; pit < pars_.size(); ++pit)
+ if (&pars_[pit] == &par)
+ break;
or even
+ pit_type pit = 0;
+ for ( ; pit < pars_.size() && &pars_[pit] != ∥ ++pit)
+ ;
wouldn't achieve the same.
Andre'
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Sat, Nov 12, 2005 at 12:36:13AM +0100, Andre Poenitz wrote:
> On Thu, Nov 10, 2005 at 03:28:42PM +0200, Martin Vermeer wrote:
> > On Wed, 2005-11-09 at 14:17 +0100, Jean-Marc Lasgouttes wrote:
> > > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
> > >
> > > Martin> So then the original patch should go in, with break added? How
> > > Martin> expensive is this with big documents? It scales linearly.
> > >
> > > The good thing is that we have really reduce the number of times
> > > getFont gets called. Nevertheless, the maybe quadratic in some cases,
> > > since getFont is called at least once per paragraph.
> > >
> > > The right solution would be to pass a DocIterator or a pit/pos pair to
> > > getFont, but this is probably a lot of work.
> > >
> > > JMarc
> >
> > So... can this go in?
>
>
> + pit_type pit = pars_.size();
> + for (pit_type it = 0; it < pars_.size(); ++it) {
> + if (&pars_[it] == &par) {
> + pit = it;
> + break;
> + }
> + }
>
>
> I just wonder whether
>
> + pit_type pit = 0;
> + for ( ; pit < pars_.size(); ++pit)
> + if (&pars_[pit] == &par)
> + break;
>
> or even
>
> + pit_type pit = 0;
> + for ( ; pit < pars_.size() && &pars_[pit] != ∥ ++pit)
> + ;
Andre, I propose you list thirteen more variants on this theme, before
Lars comes up with his boost find_if canonical solution :-)
- Martin
pgpSpZvDuNPZS.pgp
Description: PGP signature
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Andre Poenitz <[EMAIL PROTECTED]> writes: | > ParagraphList::const_iterator it = | > find_if(pars_.begin(), pars_.end(), some_functor(par)); | | Why? It's obviously more code to type, to read, and not simpler to | understand. You obviously could write out the for loop but cut the | corner in the some_functor case. ?? I cannot grok that last sentence. | [Thing would obviously different with lambda...] | | > | Of course, profiling would be interesting too :) | > | > Clearity wins. | | IMO the for loop is much clearer... break it out into a function name "find" then. It is the "find" that makes it clear we have (and have had) too many manual loops... only by close inspection is it clear if this is a disguised find, a count something else. -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
Andre Poenitz <[EMAIL PROTECTED]> writes: | + pit_type pit = 0; | + for ( ; pit < pars_.size() && &pars_[pit] != ∥ ++pit) | + ; You know... terse does not mean easy to understand. -- Lgb
Re: [New patch] Re: Fix for bug 2015: font properties propagation in environments
On Mon, Nov 14, 2005 at 05:55:46PM +0100, Lars Gullik Bjønnes wrote: > Andre Poenitz <[EMAIL PROTECTED]> writes: > > | + pit_type pit = 0; > | + for ( ; pit < pars_.size() && &pars_[pit] != ∥ ++pit) > | + ; > > You know... terse does not mean easy to understand. That's why I usually write this as pit_type pit = 0; for ( ; pit != pars_.size(); ++pit) if (&par_[pit] == &par) break; i.e. a combination of two idiom: Loop over a range, and break out if something interesting happens. Andre'
