IPRange sort order is inconsistent with coverage (superset/subset)

[gaissmai]

Hi, thanks a lot for netaddr!
Just my 5 cents: In my opinion is the sort order for IPRange.less() semi optimal:

// less returns whether r is "before" other. It is before if r.From is
// before other.From, or if they're equal, the shorter range is
// before.
func (r IPRange) less(other IPRange) bool {
	if cmp := r.From.Compare(other.From); cmp != 0 {
		return cmp < 0
	}
	return r.To.Less(other.To)
}

better would be if all subsets are sorted after their supersets. Think about this small change below:

// less returns whether r is "before" other. It is before if r.From is
// before other.From, or if they're equal, r is superset to other.
// If coveredBy is true, less must be false. subsets are always sorted "after" their supersets.
func (r IPRange) less(other IPRange) bool {
	if cmp := r.From.Compare(other.From); cmp != 0 {
		return cmp < 0
	}
       // all supersets to the left, subsets to the right
	return other.To.Less(r.To)
}

With the current sort order, the sorting of a superset to a subset is different depending if the To's OR From's are equal. Sometimes before, sometimes after, depending on the range borders and not also on the coverage.

I personally work on an interval tree for ip ranges (not only CIDRs, tries are not possible!), it is important that the supersets are always sorted on the same side of their including subsets.

Best regards
Charly

PS: the tests still pass ;)

[danderson]

This function is private and exists only to support the interval tree stuff that's part of IPSet, so it's definitely not intended to be a general implementation. That said, I think the change you propose sounds reasonable, I just need to check if our test coverage is good enough or if I have to inspect the code that uses less() for correctness after this change.

[danderson]

Okay, the IPSet logic doesn't care about this change, the only invariant it requires is that From never decreases as you traverse the sorted slice. Making the sort order a bit more predictable seems fine, and this might be a sufficiently useful definition for us to expose the sort function...