Documentation for Collection.sort(Comparator comparator) is wrong

Here's a test case

class ISO3LangComparator implements Comparator<Locale> {

    int compare(Locale locale1, Locale locale2) {
        locale1.ISO3Language <=> locale2.ISO3Language
    }
}

List<Locale> locales = [Locale.FRENCH, Locale.ENGLISH]
def sortedLocales = locales.sort(new ISO3LangComparator())

// This assertion fails
assert locales[0] == frenchLocale

Potential patch attached. Fixes Javadoc but also proposes a few new overloaded methods which allow a boolean to be provided to turn off any mutating.

Same additions could be made for the Array versions of the methods. There is also the possibility of using a Map instead of the boolean to allow sort(mutate: true) etc.

Trying to come up with a patch for unique as well

Attached a patch which hopefully should add the mutate flag to Collection.unique as well

Included test cases which should (I think) cover the new code.

Iterator.unique does not require the flag, as it wraps the Iterator wth asList before the unique call is made, so the original Collection should remain untouched.

Hope it's ok!

Sorry, I had unique(Collection, Closure, boolean) rather than unique(Collection, boolean, Closure)

Doing it boolean first means you can have:

def list = [ 1, 2, 3, 2, 1 ]
list.unique( false ) { it % 2 }

The other way round is clunky and non-ideomatic

Extended sort patch to cover arrays plus merged in unique patch from Tim. Only considered the boolean variant as I believe the map variant could be autogenerated at a later point.

Now we can consider whether we should extend this approach for:

• addAll, removeAll, retainAll (which are modify semantics by default)
  • for these I think we can refer to their copy semantics counterparts in the javadoc and vice versa (i.e. plus and findAll/grep)
  • in addition we will need a plus variant that takes an index
• reverse (which is copy semantics by default in DGM but modify in Collections)
  • another boolean would allow both semantics in this case

Also worth pointing out that for legacy reasons, we have the following slightly annoying discrepancy (I believe created for ease of implementation rather than a specific requirement):

• sort(T[] self, Closure closure) has copy semantics
• all other sort variants (without the boolean mutate flag) have modify semantics

The boolean variants allow this discrepancy to be worked around but I am wondering whether this should be considered a bug and all variations should be aligned.

There is also another consideration which arises that is related to earlier points. For methods which imply ordering and have modify semantics (such as sort), we currently perform them in place for lists (which by their nature are also ordered) but convert to a new list (indirectly giving copy semantics) for non-list collections, e.g. Set. However there are non-list collection types which are ordered, e.g. LinkedHashSet. Perhaps we should retain the modify semantics for such types. I.e. instead of using asList we should make a new asOrderedCollection method which catered for things like LinkedHashSet and possibly Queue.

A version of above patch has been applied in trunk for review. It doesn't attempt to fix the sort discrepancy or cater for LinkedHashSet etc.

The javadoc parts from trunk (and two non-contentious methods) have been merged into the 1.8 branches.

The variants with the mutate boolean (covering sort and unique) are still pending finalization and potential merging for 1.8.1.

Any updates on this?

I took a look at the doc and it seems like the unique and sort methods with boolean mutate were added with 1.8.1. Therefore I'm closing this issue.