Lars Gullik BjÃnnes wrote:

> yes + some comment.
> 
> | +using boost::prior;
> 
> which header file include next_prior.hpp?

Couldn't find out. Should I add it just in case?

 
> | +PosIterator & PosIterator::operator--()
> | +{
> | +   while (!stack_.empty()) {
> | +           {
> 
> why the strange block?
> 
> why not change the variable name or have a helper function?

Ok.

> | +PosIterator::PosIterator(ParagraphList * pl, ParagraphList::iterator
> | pit,
> | +                    lyx::pos_type pos)
> | +{
> | +   stack_.push(PosIteratorItem(pl,pit,pos));
> | +}
> 
> space between args please.
> 
> | +   *this = PosIterator(par, pos);
> 
> hmm...
> 
> operator=(PosIterator(par, pos));

Done.

> I don't know... I just find the *this = ... construct hard to read...
> | +
> | +#include "ParagraphList_fwd.h"
> | +
> | +#include <boost/scoped_ptr.hpp>
> 
> Is the scoped_ptr used here?

No, you're right.
 
> I wonder if there is a way, so that we don't need to have those
> XXX_iterator_(begin|end) functions.
> 
> but ok for now.

what about a pos_iterator_begin(Buffer &)? in PosIterator.[Ch]

> | +PosIterator::PosIterator(ParIterator & parit, lyx::pos_type pos)
> | +{
> 
> Why have this here? and not int he positerator.C file?

Because I need ParPosition which is not defined in the header file. Should I
put it on the header?

> why do they have to be friends?

Because I need private members of PosIterator in the
PosIterator(Pariterator, pos) constructor. What should I be doing instead?

Regards, Alfredo


Reply via email to