Use alphanumeric and nest-aware sorting for VisitOrder#createByName (#36)

Author: NebelNidas

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added `MappingFormat#features()` to allow for more fine-grained programmatic querying of format capabilities
 - Added tests to validate our writer outputs against 3rd-party readers
 - Overhauled the internal `ColumnFileReader` to behave more consistently
+- Made `VisitOrder#createByName` use alphanumeric and nest-aware sorting
 - Made handling of the `NEEDS_MULTIPLE_PASSES` flag more consistent, reducing memory usage in a few cases
 - Made some internal methods in Enigma and TSRG readers actually private
 - Made all writers for formats which can't represent empty destination names skip such elements entirely, unless mapped child elements are present

build.gradle

@@ -44,7 +44,8 @@ allprojects {
 	spotless {
 		java {
 			licenseHeaderFile(rootProject.file("HEADER")).yearSeparator(", ")
-			targetExclude 'src/test/java/net/fabricmc/mappingio/lib/**/*.java'
+			targetExclude 'src/test/java/net/fabricmc/mappingio/lib/**/*.java',
+			              'src/main/java/net/fabricmc/mappingio/tree/AlphanumericComparator.java'
 		}
 	}
 

src/main/java/net/fabricmc/mappingio/tree/AlphanumericComparator.java

@@ -0,0 +1,148 @@
+// Copied from https://github.com/sawano/alphanumeric-comparator/blob/5756d78617d411fbda4c51fe13d410c85392e737/src/main/java/se/sawano/java/text/AlphanumericComparator.java
+
+/*
+ * Copyright 2014 Daniel Sawano
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.mappingio.tree;
+
+import static java.nio.CharBuffer.wrap;
+import static java.util.Objects.requireNonNull;
+
+import java.nio.CharBuffer;
+import java.text.Collator;
+import java.util.Comparator;
+import java.util.Locale;
+
+class AlphanumericComparator implements Comparator<CharSequence> {
+	private final Collator collator;
+
+	/**
+	 * Creates a comparator that will use lexicographical sorting of the non-numerical parts of the compared strings.
+	*/
+	AlphanumericComparator() {
+		collator = null;
+	}
+
+	/**
+	 * Creates a comparator that will use locale-sensitive sorting of the non-numerical parts of the compared strings.
+	*
+	* @param locale The locale to use.
+	*/
+	AlphanumericComparator(Locale locale) {
+		this(Collator.getInstance(requireNonNull(locale)));
+	}
+
+	/**
+	 * Creates a comparator that will use the given collator to sort the non-numerical parts of the compared strings.
+	*
+	* @param collator The collator to use.
+	*/
+	AlphanumericComparator(Collator collator) {
+		this.collator = requireNonNull(collator);
+	}
+
+	@Override
+	public int compare(CharSequence s1, CharSequence s2) {
+		CharBuffer b1 = wrap(s1);
+		CharBuffer b2 = wrap(s2);
+
+		while (b1.hasRemaining() && b2.hasRemaining()) {
+			moveWindow(b1);
+			moveWindow(b2);
+			int result = compare(b1, b2);
+
+			if (result != 0) {
+				return result;
+			}
+
+			prepareForNextIteration(b1);
+			prepareForNextIteration(b2);
+		}
+
+		return s1.length() - s2.length();
+	}
+
+	private void moveWindow(CharBuffer buffer) {
+		int start = buffer.position();
+		int end = buffer.position();
+		boolean isNumerical = isDigit(buffer.get(start));
+
+		while (end < buffer.limit() && isNumerical == isDigit(buffer.get(end))) {
+			++end;
+
+			if (isNumerical && (start + 1 < buffer.limit()) && isZero(buffer.get(start)) && isDigit(buffer.get(end))) {
+				++start; // trim leading zeros
+			}
+		}
+
+		buffer.position(start).limit(end);
+	}
+
+	private int compare(CharBuffer b1, CharBuffer b2) {
+		if (isNumerical(b1) && isNumerical(b2)) {
+			return compareNumerically(b1, b2);
+		}
+
+		return compareAsStrings(b1, b2);
+	}
+
+	private boolean isNumerical(CharBuffer buffer) {
+		return isDigit(buffer.charAt(0));
+	}
+
+	private boolean isDigit(char c) {
+		if (collator == null) {
+			int intValue = (int) c;
+			return intValue >= 48 && intValue <= 57;
+		}
+
+		return Character.isDigit(c);
+	}
+
+	private int compareNumerically(CharBuffer b1, CharBuffer b2) {
+		int diff = b1.length() - b2.length();
+
+		if (diff != 0) {
+			return diff;
+		}
+
+		for (int i = 0; i < b1.remaining() && i < b2.remaining(); ++i) {
+			int result = Character.compare(b1.charAt(i), b2.charAt(i));
+
+			if (result != 0) {
+				return result;
+			}
+		}
+
+		return 0;
+	}
+
+	private void prepareForNextIteration(CharBuffer buffer) {
+		buffer.position(buffer.limit()).limit(buffer.capacity());
+	}
+
+	private int compareAsStrings(CharBuffer b1, CharBuffer b2) {
+		if (collator != null) {
+			return collator.compare(b1.toString(), b2.toString());
+		}
+
+		return b1.toString().compareTo(b2.toString());
+	}
+
+	private boolean isZero(char c) {
+		return c == '0';
+	}
+}

src/main/java/net/fabricmc/mappingio/tree/VisitOrder.java

@@ -20,6 +20,7 @@
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Locale;
 
 import org.jetbrains.annotations.Nullable;
 
@@ -33,6 +34,9 @@
 
 /**
  * Visitation order configuration for {@link MappingTreeView#accept(net.fabricmc.mappingio.MappingVisitor, VisitOrder)}.
+ *
+ * @apiNote The exposed comparison methods aim to produce the most human-friendly output,
+ * their sorting order is not guaranteed to be stable across library versions unless specified otherwise.
  */
 public final class VisitOrder {
 	private VisitOrder() {
@@ -44,6 +48,11 @@ public static VisitOrder createByInputOrder() {
 		return new VisitOrder();
 	}
 
+	/**
+	 * Sorts classes by their source name, members by source name and descriptor, and locals by lv- and lvt-index.
+	 *
+	 * @apiNote The sorting order is not guaranteed to be stable across library versions.
+	 */
 	public static VisitOrder createByName() {
 		return new VisitOrder()
 				.classesBySrcName()
@@ -65,6 +74,10 @@ public VisitOrder classesBySrcName() {
 		return classComparator(compareBySrcName());
 	}
 
+	public VisitOrder classesBySrcNameShortFirst() {
+		return classComparator(compareBySrcNameShortFirst());
+	}
+
 	public VisitOrder fieldComparator(Comparator<FieldMappingView> comparator) {
 		this.fieldComparator = comparator;
 
@@ -106,11 +119,16 @@ public VisitOrder methodVarComparator(Comparator<MethodVarMappingView> comparato
 	}
 
 	public VisitOrder methodVarsByLvtRowIndex() {
-		return methodVarComparator(Comparator.comparingInt(MethodVarMappingView::getLvIndex).thenComparingInt(MethodVarMappingView::getLvtRowIndex));
+		return methodVarComparator(Comparator
+				.comparingInt(MethodVarMappingView::getLvIndex)
+				.thenComparingInt(MethodVarMappingView::getLvtRowIndex));
 	}
 
 	public VisitOrder methodVarsByLvIndex() {
-		return methodVarComparator(Comparator.comparingInt(MethodVarMappingView::getLvIndex).thenComparingInt(MethodVarMappingView::getStartOpIdx));
+		return methodVarComparator(Comparator
+				.comparingInt(MethodVarMappingView::getLvIndex)
+				.thenComparingInt(MethodVarMappingView::getStartOpIdx)
+				.thenComparingInt(MethodVarMappingView::getEndOpIdx));
 	}
 
 	public VisitOrder methodsFirst(boolean methodsFirst) {
@@ -141,10 +159,16 @@ public VisitOrder methodVarsFirst() {
 		return methodVarsFirst(true);
 	}
 
-	// customization helpers
+	// customization helpers (not guaranteed to be stable across versions)
 
 	public static <T extends ElementMappingView> Comparator<T> compareBySrcName() {
-		return (a, b) -> compare(a.getSrcName(), b.getSrcName());
+		return (a, b) -> {
+			if (a instanceof ClassMappingView || b instanceof ClassMappingView) {
+				return compareNestaware(a.getSrcName(), b.getSrcName(), false);
+			} else {
+				return compare(a.getSrcName(), b.getSrcName());
+			}
+		};
 	}
 
 	public static <T extends MemberMappingView> Comparator<T> compareBySrcNameDesc() {
@@ -156,41 +180,58 @@ public static <T extends MemberMappingView> Comparator<T> compareBySrcNameDesc()
 	}
 
 	public static <T extends ElementMappingView> Comparator<T> compareBySrcNameShortFirst() {
-		return (a, b) -> compareShortFirst(a.getSrcName(), b.getSrcName());
+		return (a, b) -> {
+			if (a instanceof ClassMappingView || b instanceof ClassMappingView) {
+				return compareNestaware(a.getSrcName(), b.getSrcName(), true);
+			} else {
+				return compareShortFirst(a.getSrcName(), b.getSrcName());
+			}
+		};
+	}
+
+	public static <T extends MemberMappingView> Comparator<T> compareBySrcNameDescShortFirst() {
+		return (a, b) -> {
+			int cmp = compareShortFirst(a.getSrcName(), b.getSrcName());
+
+			return cmp != 0 ? cmp : compare(a.getSrcDesc(), b.getSrcDesc());
+		};
 	}
 
 	public static int compare(@Nullable String a, @Nullable String b) {
 		if (a == null || b == null) return compareNullLast(a, b);
 
-		return a.compareTo(b);
+		return ALPHANUM.compare(a, b);
+	}
+
+	public static int compare(String a, int startA, int endA, String b, int startB, int endB) {
+		return ALPHANUM.compare(a.substring(startA, endA), b.substring(startB, endB));
 	}
 
 	public static int compareShortFirst(@Nullable String a, @Nullable String b) {
 		if (a == null || b == null) return compareNullLast(a, b);
 
 		int cmp = a.length() - b.length();
 
-		return cmp != 0 ? cmp : a.compareTo(b);
+		return cmp != 0 ? cmp : ALPHANUM.compare(a, b);
 	}
 
 	public static int compareShortFirst(String a, int startA, int endA, String b, int startB, int endB) {
 		int lenA = endA - startA;
 		int ret = Integer.compare(lenA, endB - startB);
 		if (ret != 0) return ret;
 
-		for (int i = 0; i < lenA; i++) {
-			char ca = a.charAt(startA + i);
-			char cb = b.charAt(startB + i);
-
-			if (ca != cb) {
-				return ca - cb;
-			}
-		}
+		return ALPHANUM.compare(a.substring(startA, endA), b.substring(startB, endB));
+	}
 
-		return 0;
+	public static int compareNestaware(@Nullable String a, @Nullable String b) {
+		return compareNestaware(a, b, false);
 	}
 
 	public static int compareShortFirstNestaware(@Nullable String a, @Nullable String b) {
+		return compareNestaware(a, b, true);
+	}
+
+	private static int compareNestaware(@Nullable String a, @Nullable String b, boolean shortFirst) {
 		if (a == null || b == null) {
 			return compareNullLast(a, b);
 		}
@@ -201,8 +242,11 @@ public static int compareShortFirstNestaware(@Nullable String a, @Nullable Strin
 			int endA = a.indexOf('$', pos);
 			int endB = b.indexOf('$', pos);
 
-			int ret = compareShortFirst(a, pos, endA >= 0 ? endA : a.length(),
-					b, pos, endB >= 0 ? endB : b.length());
+			int ret = shortFirst
+					? compareShortFirst(a, pos, endA >= 0 ? endA : a.length(),
+							b, pos, endB >= 0 ? endB : b.length())
+					: compare(a, pos, endA >= 0 ? endA : a.length(),
+							b, pos, endB >= 0 ? endB : b.length());
 
 			if (ret != 0) {
 				return ret;
@@ -226,7 +270,7 @@ public static int compareNullLast(@Nullable String a, @Nullable String b) {
 		} else if (b == null) { // only b null
 			return -1;
 		} else { // neither null
-			return a.compareTo(b);
+			return ALPHANUM.compare(a, b);
 		}
 	}
 
@@ -269,6 +313,7 @@ public boolean isMethodVarsFirst() {
 		return methodVarsFirst;
 	}
 
+	private static final AlphanumericComparator ALPHANUM = new AlphanumericComparator(Locale.ROOT);
 	private Comparator<ClassMappingView> classComparator;
 	private Comparator<FieldMappingView> fieldComparator;
 	private Comparator<MethodMappingView> methodComparator;