xsltc cloneIterator() question

[John Howard <Jo...@schemasoft.com>]

Hi all,

In the course of debugging
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=4905 I've collected a few
issues about the cloneIterator() codepath and am posting them here for
anyone out there to review (Morten?).

I did a quick search to see if most NodeIterators reset() the clone they
were making before returning it from cloneIterator() (in light of the fix to
#4905 I'm not sure how to determine which is incorrect):

AbsoluteIterator:        return clone.reset();
CurrentNodeListIterator: return clone.reset();
PrecedingIterator:       return clone.reset();
AncestorIterator:        return clone.reset();
NodeValueIterator:       return clone.reset();
FilteredStepIterator:    return clone.resetPosition();
FilterIterator:          return clone.reset();
MatchingIterator:        return clone;
AxisIterator:            return clone;
NodeValueIterator:       return clone.reset();
NthIterator:             return new NthIterator(....);
UnionIterator:           return clone.reset();

The following I consider suspicious on the basis that during cloning the
state of the clone's _isRestartable should be changed, not the original
object's flag:

PrecedingIterator:       this._isRestartable = false;
AncestorIterator:        this._isRestartable = false;
StepIterator:            this._isRestartable = false;
UnionIterator:           this._isRestartable = false;

The following I consider suspicious on the basis that setStartNode(int) has
a codepath that calls NodeIteratorBase.resetPosition() but reset() has no
such path. I have noticed that the absence of the call to resetPosition() in
reset() caused a cloned TypedChildrenIterator to have a different state
(namely, _position was out of sync - if it matters).

TypedChildrenIterator:   return this;
UnionIterator:           return(this);

I post these things here because they don't look right; I don't know if
there are open bugs for any of this so I didn't post bug reports. Are they
mistakes? Should I submit the patches?

Cheers,

john

Re: xsltc cloneIterator() question

[Morten Jorgensen <mo...@sun.com>]

Hello again John,

I've added some comments below.

> In the course of debugging
> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=4905 I've collected a few
> issues about the cloneIterator() codepath and am posting them here for
> anyone out there to review (Morten?).
> 
> I did a quick search to see if most NodeIterators reset() the clone they
> were making before returning it from cloneIterator() (in light of the fix to
> #4905 I'm not sure how to determine which is incorrect):

Node iterators should be reset when cloned, and resetting an iterator should
normally also reset its position. Some iterators (such as the MatchingIterator)
are just wrappers around other iterators and do not really have any position
to reset, so the call to reset() is intentionally omitted.

> The following I consider suspicious on the basis that during cloning the
> state of the clone's _isRestartable should be changed, not the original
> object's flag:
> 
> PrecedingIterator:       this._isRestartable = false;
> AncestorIterator:        this._isRestartable = false;
> StepIterator:            this._isRestartable = false;
> UnionIterator:           this._isRestartable = false;

(Note that the _isRestartable flag is set in the source iterator _before_
it is cloned, so the flag is set to false in both source and clone.)

Cloned iterators are used for variables and parameters. The cloneIterator()
method is called when the variable or parameter is read to ensure that the
variable or parameter's value does not change. The value of global parameters
and variables never change, so it is OK to tag them as not restartable. 
Iterators stored in local parameters and variables are not re-used but are
rather overwritten every time their values change, so marking them as non-
restartable should not make any difference.

The only exception that I can think of is with the StepIterator that relies
on two underlying iterators (left and right). The 'left' iterator is used
to produce start nodes for the 'right' iterator, so the 'right' iterator
should always be restartable.

> The following I consider suspicious on the basis that setStartNode(int) has
> a codepath that calls NodeIteratorBase.resetPosition() but reset() has no
> such path. I have noticed that the absence of the call to resetPosition() in
> reset() caused a cloned TypedChildrenIterator to have a different state
> (namely, _position was out of sync - if it matters).
> 
> TypedChildrenIterator:   return this;
> UnionIterator:           return(this);

This does matter. Well spotted. I'll update the code in these two nodes
as needed (by adding the call to resetPosition()).

> I post these things here because they don't look right; I don't know if
> there are open bugs for any of this so I didn't post bug reports. Are they
> mistakes? Should I submit the patches?

I don't think there is any need to. I'll update the two iterators that are
lacking the call to resetPosition().

Morten