refactor: push down the equality checking code in EqualsVisitor #1853
Conversation
monperrus
force-pushed
the
refactor-biscan
branch
from
8fb1c2a to
b72b901
Compare
| return isNotEqual; | ||
| } | ||
| /** This method is called to compare `element` and `other` when traversing two trees in parallel.*/ | ||
| public abstract boolean biScan(CtElement element, CtElement other); |
There was a problem hiding this comment.
CtAbstractBiScanner sounds now like general purpose bi-scanner. Do I understood it well? If yes, then please change comments here (it is no more about "compare") and change return value from boolean to void
WDYT?
There was a problem hiding this comment.
CtAbstractBiScanner sounds now like general purpose bi-scanner. Do I understood it well?
yes, correct.
I agree with you, done.
| return isNotEqual; | ||
| } | ||
|
|
||
| stack.push(other); |
There was a problem hiding this comment.
Could we move stack.push(other); and stack,pop() somewhere into CtBiScannerDefault? I looks like implementation of CtBiScannerDefault in this PR is useless without calls of stack.push(other); and stack,pop(). Because CtBiScannerDefault calls stack.peek(), but nobody pushes data there.
WDYT?
There was a problem hiding this comment.
Good point. Done.
|
It would be probably more readable if all biScan methods would return void instead of boolean which is on most places ignored. The only place where return value is actually used is public class EqualsVisitor extends CtBiScannerDefault {
public static boolean equals(CtElement element, CtElement other) {
return !new EqualsVisitor().biScan(element, other);
}which might be replaced by public static boolean equals(CtElement element, CtElement other) {
return new EqualsVisitor().isEqual(element, other);
}
...
boolean isEqual(CtElement element, CtElement other) {
biScan(element, other);
return !isNotEqual;
} |
| } | ||
|
|
||
| protected boolean biScan(CtRole role, Collection<? extends CtElement> elements, Collection<? extends CtElement> others) { | ||
| for (Iterator<? extends CtElement> firstIt = elements.iterator(), secondIt = others.iterator(); (firstIt.hasNext()) && (secondIt.hasNext());) { |
There was a problem hiding this comment.
I guess this code does not work for Set since beginning, because you cannot assure that elements in 2 Sets are iterated in consistent pairs. I wonder why it works ... WDYT?
|
for the beauty of it, added a test to do a parallel biscan visit of the same tree. |
|
How does biscan work on Set of children elements? For example CtType#superInterfaces? How is there assured that items from both Sets are paired correctly? |
|
This is a question we discussed when we introduced it. A strong assumption -- even a a requirement -- is we only use implementations of |
|
the design here was quite broken. this PR fixes it.