Optimize CtElement.compareTo()

[leventov]

it should be based on BiScanner.

Here is my version of CompareToScanner, it is based on the different version of BiScanner than commited into the repo, but still that might help:

/**
 * Copyright (C) 2006-2015 INRIA and contributors
 * Spoon - http://spoon.gforge.inria.fr/
 *
 * This software is governed by the CeCILL-C License under French law and
 * abiding by the rules of distribution of free software. You can use, modify
 * and/or redistribute the software under the terms of the CeCILL-C license as
 * circulated by CEA, CNRS and INRIA at http://www.cecill.info.
 *
 * This program is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE. See the CeCILL-C License for more details.
 *
 * The fact that you are presently reading this means that you have had
 * knowledge of the CeCILL-C license and that you accept its terms.
 */
package spoon.reflect.visitor;

import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtNamedElement;
import spoon.reflect.reference.CtReference;

import java.util.Arrays;
import java.util.Collection;
import java.util.List;

import static spoon.reflect.visitor.BiScanner.ScanResult.CONTINUE;
import static spoon.reflect.visitor.BiScanner.ScanResult.TERMINATE;

public class CompareToScanner extends BiScanner {

    private int result = 0;

    public int getResult() {
        return result;
    }

    @Override
    public ScanResult inspectElements(CtElement left, CtElement right) {
        if (left == right) {
            // Including both are nulls.
            return CONTINUE;
        }
        if (left == null) {
            result = -1;
            return TERMINATE;
        }
        if (right == null) {
            result = 1;
            return TERMINATE;
        }
        if (left instanceof CtNamedElement && right instanceof CtNamedElement) {
            String leftName = ((CtNamedElement) left).getSimpleName();
            String rightName = ((CtNamedElement) right).getSimpleName();
            result = leftName.compareTo(rightName);
            if (result != 0) {
                return TERMINATE;
            }
        }
        if (left instanceof CtReference && right instanceof CtReference) {
            String leftName = ((CtReference) left).getSimpleName();
            String rightName = ((CtReference) right).getSimpleName();
            result = leftName.compareTo(rightName);
            if (result != 0) {
                return TERMINATE;
            }
        }
        if (left.getClass() != right.getClass()) {
            return compareClasses(left.getClass(), right.getClass());
        }
        return super.inspectElements(left, right);
    }

    @Override
    public ScanResult inspectCollections(Collection<?> lefts, Collection<?> rights) {
        if (lefts == rights) {
            // Including both are nulls.
            return CONTINUE;
        }
        if (lefts == null) {
            result = -1;
            return TERMINATE;
        }
        if (rights == null) {
            result = 1;
            return TERMINATE;
        }
        if ((lefts instanceof List && rights instanceof List)) {
            return traverse((List<?>) lefts, (List<?>) rights);
        }
        // Set + List, Set + Set, or any other collections -- compare just by size.
        result = lefts.size() - rights.size();
        return result == 0 ? CONTINUE : TERMINATE;
    }

    @Override
    public ScanResult inspectValues(Object left, Object right) {
        if (left == right) {
            // Including both are nulls.
            return CONTINUE;
        }
        if (left == null) {
            result = -1;
            return TERMINATE;
        }
        if (right == null) {
            result = 1;
            return TERMINATE;
        }
        if (left.getClass() != right.getClass()) {
            return compareClasses(left.getClass(), right.getClass());
        }
        if (left.getClass().isArray()) {
            String leftS = Arrays.deepToString(new Object[]{left});
            String rightS = Arrays.deepToString(new Object[]{right});
            result = leftS.compareTo(rightS);
        } else {
            if (!(left instanceof Comparable)) {
                throw new IllegalStateException(
                        "Values could be java (boxed) primitives, strings, enum constants or arrays, not "
                                + left.getClass());
            }
            result = ((Comparable) left).compareTo(right);
        }
        return result == 0 ? CONTINUE : TERMINATE;
    }

    private ScanResult compareClasses(Class<?> leftClass, Class<?> rightClass) {
        result = leftClass.getName().compareTo(rightClass.getName());
        if (result == 0) {
            throw new IllegalStateException("Two not equal classes have the same name " + leftClass.getName());
        }
        return TERMINATE;
    }
}

[monperrus]

duplicate of #636?

[tdurieux]

From my point of view that makes absolute no sense to be able to compare two CtElements. What does it mean?

Only the position of two elements are comparable. I thing it is better the replace the TreeSet/TreeMap by HashSet/HashMap or use the position of the element in the compareTo but what is the position of a new CtElement...

[leventov]

@tdurieux I totally support this, just trying to optimize compareTo() for the current set or requirements. Going through the whole codebase and removing all TreeMaps/TreeSets is not something that could be PR'ed from outside. But if you or @monperrus or @GerardPaligot do this I would be happy.

[monperrus]

I also agree.

Two solutions:

1; Removing Comparable Pb: huge change in the code base (all TreeSet), breaks a lot of client code

1. changing compareTo to a simpler one: Advantage:
2. performance Pb:
3. some changes in the tests that depend on that order (they should not but there are some)
4. may break clients that also implicitly rely on this order

I prefer the second solution, esp. if we can clearly measure the performance gain.

[monperrus]

Closing this one, we have #636