Jean-Marc Lasgouttes wrote: > > Alfredo Braunstein <[EMAIL PROTECTED]> > writes: >> This patch removes some code duplication in DocIterator (in particular >> removes forwardPosNoDescend that if nothing else, has an ugly name), by >> moving the part of forwardPos that acts on flat cursorslices to >> CursorSlice::operator++. Comments? > > Well, Andre's first comment was "it's from Alfredo, it is correct". > However, after we forced ourselves to actually read the code, we are > unsure that the patch is a good thing. > > forwardPosNoDescend may be ugly, but it is quite explicit. ++cur.pit()
I take it you mean ++cur.top(). Would it help to call it CursorSlice::forwardPos ? > requires much more thinking to be sure of what it means. The original > version in particular can go outside of insets and continue from > there, which is not the case of the CursorSlice one. If fact it is rather weird, and the "proof" is that we don't need this feature at all in the code. Without that "feature" (and no caller code uses it currently), the thing only moves on a flat cursorslice, I find counterintuitive to have it on DocIterator. > Also concerning setFont, I am not sure we want to use CursorSlice as > parameter, although it makes sense technically. Things will get soon > annoying if for each function one has to wonder whether to use a full > cursor or a slice... I have been advocating the contrary from the beginning ;-) Restricting the data you have available make the code more robust, I find. IMO it's better to use what's strictly necessary and no more... In that way you don't have to guess if the thing acts recursively on all inner depths or just on the same level: it's written on the function signature. > All in all this looks like purely cosmetic, but I am do not really > like the end result than the starting point. FWIW, I was starting some new cool stuff (!) and I needed ++top, then I found out about this NoDescend business and got taken by cleaning fever. Of course it is not very important, I just thought it lead to cleaner code (for instance, a two-screenful function get split into two functions of one screenful each, the total line-count is negative... A/
