review: Add mechanism to sort compilation units #1938
Conversation
| @Override | ||
| public int compare(CompilationUnitDeclaration o1, CompilationUnitDeclaration o2) { | ||
| int seed = 0; | ||
| if (System.getenv("SPOON_SEED_CU_COMPARATOR") != null) { |
There was a problem hiding this comment.
copied from javadoc of System#getenv
If a security manager exists, its checkPermission method is called with a RuntimePermission("getenv."+name) permission. This may result in a SecurityException being thrown.
It would be cleaner to try catch security exception here.
| @@ -430,8 +430,11 @@ protected boolean buildUnitsAndModel(JDTBuilder jdtBuilder, SpoonFolder sourcesF | |||
|
|
|||
| protected void buildModel(CompilationUnitDeclaration[] units) { | |||
| JDTTreeBuilder builder = new JDTTreeBuilder(factory); | |||
| List<CompilationUnitDeclaration> unitList = Arrays.asList(units); | |||
| unitList.sort(new CompilationUnitComparator()); | |||
There was a problem hiding this comment.
Is it intentional that this sort sorts the origin units array? It actually modifies the array sent from outside. I have checked who actually calls it and it looks like it is actually no problem, but it would be safer to sort a copy of that array.
WDYT?
There was a problem hiding this comment.
You're right: I wasn't aware that Arrays.asList was keeping up the link with the original array. Good thing to know 👍
|
Java code is OK for me. |
|
Interesting. Two comments:
|
No as I said in #1929 that we should have always the same order but we should test that we are not dependent of the order: a way to do that is to randomize the order when testing.
OK with that change, I'll do it |
Before I do the change, just to be clear, you want me to integrate my test with |
|
Yes. We can add a method It seems to me that adding a new Travis job for such a specific need and rare bug is somehow overkill, because it is run on all PRs. You suggested a Jenkins job which I tend to prefer. WDYT? |
OK I'll do it that way
Jenkins does not provide an immediate feedback, however I agree it's overkill to run this test on all PRs commits: a middle way might be to put it only for commits on master. |
|
then we can have a Gakoci job for master.
|
|
I changed the chore script in db939a1 so it's executed in Travis only for master commits. |
|
what about adding a test case? |
| @@ -328,6 +328,7 @@ public void testSpecPackage() throws Exception { | |||
| officialPackages.add("spoon.reflect.visitor"); | |||
| officialPackages.add("spoon.reflect"); | |||
| officialPackages.add("spoon.support.comparator"); | |||
| officialPackages.add("spoon.support.comparator.cu"); | |||
There was a problem hiding this comment.
seems overkill to add a new package for those two small classes? spoon.support.comparator would be enough?
…Add some doc in bash file.
|
@monperrus it's ready for me. |
|
thanks! |
This PR should fix #1929