Permalink
Browse files

Fixes #24 - Descendant iterator (and by proxy, FilterIterator) broken

after remove().
Includes a bunch more test cases, and fixes some redundancy in
Element/Document.
  • Loading branch information...
1 parent 932cfb2 commit e270fd04a335dc6a3c431b4827f26411b03e873f @rolfl rolfl committed Oct 11, 2011
View
@@ -171,6 +171,11 @@ For instructions on how to build JDOM, please view the README.txt file.
<target name="compile.core" depends="prepare"
description="Compiles the core source code">
+ <!-- Copy resources and stuff - everything except Java code -->
+ <copy todir="${core.build}" >
+ <fileset dir="${core.src}" excludes="**/*.java" />
+ </copy>
+
<javac srcdir="${core.src}"
destdir="${core.build}"
debug="${compile.debug}"
@@ -187,7 +192,7 @@ For instructions on how to build JDOM, please view the README.txt file.
<!-- Copy resources and stuff - everything except Java code -->
<copy todir="${samples.build}" >
- <fileset dir="${samples.src}" excludes="*.java" />
+ <fileset dir="${samples.src}" excludes="**/*.java" />
</copy>
<javac srcdir="${samples.src}"
@@ -206,7 +211,7 @@ For instructions on how to build JDOM, please view the README.txt file.
<!-- Copy resources and stuff - everything except Java code -->
<copy todir="${contrib.build}" >
- <fileset dir="${contrib.src}" excludes="*.java" />
+ <fileset dir="${contrib.src}" excludes="**/*.java" />
</copy>
<javac srcdir="${contrib.src}"
@@ -225,7 +230,7 @@ For instructions on how to build JDOM, please view the README.txt file.
<!-- Copy resources and stuff - everything except Java code -->
<copy todir="${junit.build}" >
- <fileset dir="${junit.src}" excludes="*.java" />
+ <fileset dir="${junit.src}" excludes="**/*.java" />
</copy>
<javac srcdir="${junit.src}"
@@ -330,10 +335,9 @@ For instructions on how to build JDOM, please view the README.txt file.
<delete dir="${junit.xml}" />
<mkdir dir="${junit.xml}" />
- <!-- this may already be set true in the cobertura task. -->
- <property name="forkjunit" value="false" />
-
- <junit fork="${forkjunit}" forkmode="once" haltonerror="false" haltonfailure="false"
+ <!-- We need to fork to get the resources on the classpath -->
+ <!-- Otherwise we rely on ant's ClassLoader which does not do resources nicely -->
+ <junit fork="true" forkmode="once" haltonerror="false" haltonfailure="false"
failureproperty="junit.failed" printsummary="true" timeout="100000"
showoutput="true" includeantruntime="true" >
@@ -345,10 +349,13 @@ For instructions on how to build JDOM, please view the README.txt file.
<batchtest haltonerror="false" haltonfailure="false"
failureproperty="junit.failed" todir="${junit.xml}" >
- <fileset dir="${junit.src}">
- <include name="**/Test*.java"/>
- <exclude name="**/generate/**" />
- </fileset>
+ <sort>
+ <name />
+ <fileset dir="${junit.src}">
+ <include name="**/Test*.java"/>
+ <exclude name="**/generate/**" />
+ </fileset>
+ </sort>
</batchtest>
</junit>
@@ -57,7 +57,6 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
import java.util.*;
import org.jdom2.Content;
-import org.jdom2.Element;
import org.jdom2.Parent;
/**
@@ -69,32 +68,41 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
*/
class DescendantIterator implements Iterator<Content> {
- private Iterator<Content> iterator;
- private Iterator<Content> nextIterator;
- private List<Iterator<Content>> stack = new ArrayList<Iterator<Content>>();
+ private ArrayDeque<ListIterator<Content>> stack = new ArrayDeque<ListIterator<Content>>();
+ ListIterator<Content> current = null;
+ ListIterator<Content> following = null;
/**
* Iterator for the descendants of the supplied object.
*
* @param parent document or element whose descendants will be iterated
*/
DescendantIterator(Parent parent) {
- if (parent == null) {
- throw new IllegalArgumentException("parent parameter was null");
- }
- this.iterator = parent.getContent().iterator();
+ // can trust that parent is not null, DescendantIterator is package-private.
+ current = parent.getContent().listIterator();
}
/**
- * Returns true> if the iteration has more {@link Content} descendants.
+ * Returns <b>true</b> if the iteration has more {@link Content} descendants.
*
* @return true is the iterator has more descendants
*/
@Override
public boolean hasNext() {
- if (iterator != null && iterator.hasNext()) return true;
- if (nextIterator != null && nextIterator.hasNext()) return true;
- if (stackHasAnyNext()) return true;
+ if (following != null && following.hasNext()) {
+ // the last content returned has un-processed child content
+ return true;
+ }
+ if (current.hasNext()) {
+ // the last content has un-iterated siblings.
+ return true;
+ }
+ for (Iterator<Content> it : stack) {
+ if (it.hasNext()) {
+ // an ancestor has un-iterated siblings.
+ return true;
+ }
+ }
return false;
}
@@ -105,34 +113,35 @@ public boolean hasNext() {
*/
@Override
public Content next() {
- if (!hasNext()) {
- throw new NoSuchElementException();
- }
-
- // If we need to descend, go for it and record where we are.
- // We do the shuffle here on the next next() call so remove() is easy
- // to code up.
- if (nextIterator != null) {
- push(iterator);
- iterator = nextIterator;
- nextIterator = null;
- }
-
- // If this iterator is finished, try moving up the stack
- while (!iterator.hasNext()) {
- if (stack.size() > 0) {
- iterator = pop();
- }
- else {
- throw new NoSuchElementException("Somehow we lost our iterator");
+ Content ret = null;
+ if (following != null && following.hasNext()) {
+ // The last returned content is a parent with unprocessed content
+ ret = following.next();
+ stack.push(current);
+ current = following;
+ } else if (current.hasNext()) {
+ // the last content returned has un-iterated siblings.
+ ret = current.next();
+ } else {
+ // check up the ancestry for the next unprocessed sibling...
+ while (ret == null && !stack.isEmpty()) {
+ // while we go we pop the stack.
+ current = stack.pop();
+ if (current.hasNext()) {
+ ret = current.next();
+ }
}
}
-
- Content child = iterator.next();
- if (child instanceof Element) {
- nextIterator = ((Element)child).getContent().iterator();
+ if (ret == null) {
+ throw new NoSuchElementException("Iterated off the end of the " +
+ "DescendantIterator");
}
- return child;
+ if (ret instanceof Parent) {
+ following = ((Parent)ret).getContent().listIterator();
+ } else {
+ following = null;
+ }
+ return ret;
}
/**
@@ -143,29 +152,8 @@ public Content next() {
*/
@Override
public void remove() {
- iterator.remove();
- }
-
- private Iterator<Content> pop() {
- int stackSize = stack.size();
- if (stackSize == 0) {
- throw new NoSuchElementException("empty stack");
- }
- return stack.remove(stackSize - 1);
- }
-
- private void push(Iterator<Content> itr) {
- stack.add(itr);
+ current.remove();
+ following = null;
}
- private boolean stackHasAnyNext() {
- int size = stack.size();
- for (int i = 0; i < size; i++) {
- Iterator<Content> itr = stack.get(i);
- if (itr.hasNext()) {
- return true;
- }
- }
- return false;
- }
}
@@ -728,7 +728,7 @@ else if (obj instanceof DocType) {
* @return an iterator to walk descendants within a filter
*/
@Override
- public <F extends Content> Iterator<F> getDescendants(Filter<F> filter) {
+ public <F extends Content> Iterator<F> getDescendants(final Filter<F> filter) {
return new FilterIterator<F>(new DescendantIterator(this), filter);
}
@@ -1479,8 +1479,7 @@ private void readObject(final ObjectInputStream in)
*/
@Override
public <F extends Content> Iterator<F> getDescendants(final Filter<F> filter) {
- final Iterator<Content> iterator = new DescendantIterator(this);
- return new FilterIterator<F>(iterator, filter);
+ return new FilterIterator<F>(new DescendantIterator(this), filter);
}
@@ -62,28 +62,34 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
*
* @author Bradley S. Huffman
*/
-class FilterIterator<T extends Content> implements Iterator<T> {
+class FilterIterator<T> implements Iterator<T> {
- private Iterator<? extends Content> iterator;
+ private Iterator<?> iterator;
private Filter<T> filter;
private T nextObject;
+ private boolean canremove = false;
- public FilterIterator(Iterator<? extends Content> iterator, Filter<T> filter) {
- if ((iterator == null) || (filter == null)) {
- throw new IllegalArgumentException("null parameter");
+ public FilterIterator(Iterator<?> iterator, Filter<T> filter) {
+ // can trust that iterator is not null, but filter may be.
+ if (filter == null) {
+ throw new NullPointerException("Cannot specify a null Filter " +
+ "for a FilterIterator");
}
this.iterator = iterator;
this.filter = filter;
}
@Override
public boolean hasNext() {
+
+ canremove = false;
+
if (nextObject != null) {
return true;
}
while (iterator.hasNext()) {
- Content obj = iterator.next();
+ Object obj = iterator.next();
T f = filter.filter(obj);
if (f != null) {
nextObject = f;
@@ -101,13 +107,19 @@ public T next() {
T obj = nextObject;
nextObject = null;
+ canremove = true;
return obj;
}
@Override
public void remove() {
- // XXX Could cause probs for sure if hasNext() is
- // called before the remove(), although that's unlikely.
+ if (!canremove) {
+ throw new IllegalStateException("remove() can only be called " +
+ "on the FilterIterator immediately after a successful " +
+ "call to next(). A call to remove() immediately after " +
+ "a call to hasNext() or remove() will also fail.");
+ }
+ canremove = false;
iterator.remove();
}
}
Oops, something went wrong.

0 comments on commit e270fd0

Please sign in to comment.