Make ColumnFileReader consume trailing column separators instead of…

… leading ones (#108) This restores the pre-36c52a5 behavior, fixing `\tc` (a comment) sometimes being interpreted as `c` (a class) in Tiny v2.
FabricMC · Aug 26, 2024 · d84108e · d84108e
1 parent a27d20a
commit d84108e
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 39 deletions.
diff --git a/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java b/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java
@@ -32,6 +32,11 @@
 @ApiStatus.Internal
 public final class ColumnFileReader implements Closeable {
 	public ColumnFileReader(Reader reader, char indentationChar, char columnSeparator) {
+		assert indentationChar != '\r';
+		assert indentationChar != '\n';
+		assert columnSeparator != '\r';
+		assert columnSeparator != '\n';
+
 		this.reader = reader;
 		this.indentationChar = indentationChar;
 		this.columnSeparator = columnSeparator;
@@ -51,7 +56,7 @@ public void close() throws IOException {
 	 * @return {@code true} if the column was read and had the expected content, {@code false} otherwise.
 	 */
 	public boolean nextCol(String expected) throws IOException {
-		return read(false, false, true, expected) != noMatch;
+		return read(false, false, true, expected) != NO_MATCH;
 	}
 
 	/**
@@ -89,52 +94,45 @@ public String peekCol(boolean unescape) throws IOException {
 	 * @param unescape Whether to unescape the read string.
 	 * @param consume Whether to advance the bufferPos.
 	 * @param stopAtNextCol Whether to only read one column.
-	 * @param expected If not {@code null}, the read string must match this exactly, otherwise we early-exit with {@link #noMatch}. Always consumes if matched.
+	 * @param expected If not {@code null}, the read string must match this exactly, otherwise we early-exit with {@link #NO_MATCH}. Always consumes if matched.
 	 *
 	 * @return {@code null} if nothing has been read (first char was EOL), otherwise the read string (may be empty).
-	 * If {@code expected} is not {@code null}, it will be returned if matched, otherwise {@link #noMatch}.
+	 * If {@code expected} is not {@code null}, it will be returned if matched, otherwise {@link #NO_MATCH}.
 	 */
 	@Nullable
 	private String read(boolean unescape, boolean consume, boolean stopAtNextCol, @Nullable String expected) throws IOException {
-		if (eol) return expected == null ? null : noMatch;
+		if (eol) return expected == null ? null : NO_MATCH;
 
 		int expectedLength = expected != null ? expected.length() : -1;
 
 		// Check if the buffer needs to be filled and if we hit EOF while doing so
 		if (expectedLength > 0 && bufferPos + expectedLength >= bufferLimit) {
-			if (!fillBuffer(expectedLength, !consume, false)) return noMatch;
+			if (!fillBuffer(expectedLength, !consume, false)) return NO_MATCH;
 		}
 
 		int start;
 		int end = this.bufferPos;
 		int firstEscaped = -1;
 		int contentCharsRead = 0;
 		int modifiedBufferPos = -1;
-		int startOffset = 0;
 		boolean readAnything = false;
 		boolean filled = true;
+		boolean isColumnSeparator = false;
 
 		readLoop: for (;;) {
 			while (end < bufferLimit) {
 				char c = buffer[end];
-				boolean isColumnSeparator = (c == columnSeparator);
-
-				// skip leading column separator
-				if (isColumnSeparator && !readAnything) {
-					startOffset = 1;
-					contentCharsRead = -1;
-				}
-
+				isColumnSeparator = (c == columnSeparator);
 				readAnything = true;
 
-				if (expected != null && contentCharsRead > -1) {
+				if (expected != null) {
 					if ((contentCharsRead < expectedLength && c != expected.charAt(contentCharsRead))
 							|| contentCharsRead > expectedLength) {
-						return noMatch;
+						return NO_MATCH;
 					}
 				}
 
-				if (c == '\n' || c == '\r' || (isColumnSeparator && stopAtNextCol && contentCharsRead > -1)) { // stop reading
+				if (c == '\n' || c == '\r' || (isColumnSeparator && stopAtNextCol)) { // stop reading
 					start = bufferPos;
 					modifiedBufferPos = end;
 
@@ -166,28 +164,36 @@ private String read(boolean unescape, boolean consume, boolean stopAtNextCol, @N
 			}
 		}
 
-		start += startOffset;
 		String ret;
 
 		if (expected != null) {
 			consume = true;
 			ret = expected;
 		} else {
-			int len = end - start;
+			int contentLength = end - start;
 
-			if (len == 0) {
+			if (contentLength == 0) {
 				ret = readAnything ? "" : null;
 			} else if (firstEscaped >= 0) {
-				ret = Tiny2Util.unescape(String.valueOf(buffer, start, len));
+				ret = Tiny2Util.unescape(String.valueOf(buffer, start, contentLength));
 			} else {
-				ret = String.valueOf(buffer, start, len);
+				ret = String.valueOf(buffer, start, contentLength);
 			}
 		}
 
 		if (consume) {
 			if (readAnything) bof = false;
+
+			if (modifiedBufferPos != -1) {
+				bufferPos = modifiedBufferPos;
+
+				// consume trailing column separator if present
+				if (isColumnSeparator && filled && fillBuffer(1, false, false)) {
+					bufferPos++;
+				}
+			}
+
 			if (!filled) eof = eol = true;
-			if (modifiedBufferPos != -1) bufferPos = modifiedBufferPos;
 
 			if (eol && !eof) { // manually check for EOF
 				int charsToRead = buffer[bufferPos] == '\r' ? 2 : 1; // 2 for \r\n, 1 for just \n
@@ -237,6 +243,15 @@ public int nextIntCol() throws IOException {
 		}
 	}
 
+	/**
+	 * Read and consume until the the start of the next line is reached, and return whether the
+	 * following {@code indent} characters match {@link #indentationChar}.
+	 *
+	 * <p>Empty lines are skipped if {@code indent} is 0.
+	 *
+	 * @param indent The number of characters to check for indentation.
+	 * @return {@code true} if the next line has the specified indentation or higher, {@code false} otherwise.
+	 */
 	public boolean nextLine(int indent) throws IOException {
 		fillLoop: do {
 			while (bufferPos < bufferLimit) {
@@ -246,9 +261,9 @@ public boolean nextLine(int indent) throws IOException {
 					if (indent == 0) { // skip empty lines if indent is 0
 						if (!fillBuffer(2, false, true)) break fillLoop;
 
-						c = buffer[bufferPos + 1];
+						char next = buffer[bufferPos + 1];
 
-						if (c == '\n' || c == '\r') { // 2+ consecutive new lines, consume first nl and retry
+						if (next == '\n' || next == '\r') { // 2+ consecutive new lines, consume first nl and retry
 							bufferPos++;
 							lineNumber++;
 							bof = false;
@@ -428,7 +443,7 @@ private boolean fillBuffer(int count, boolean preventCompaction, boolean markEof
 		return true;
 	}
 
-	private static final String noMatch = new String();
+	private static final String NO_MATCH = new String();
 	private final Reader reader;
 	private final char indentationChar;
 	private final char columnSeparator;

diff --git a/src/test/java/net/fabricmc/mappingio/TestHelper.java b/src/test/java/net/fabricmc/mappingio/TestHelper.java
@@ -93,6 +93,7 @@ public static MemoryMappingTree createTestTree() {
 		visitMethodArg(tree, dstNs);
 		visitMethodVar(tree, dstNs);
 		visitInnerClass(tree, 1, dstNs);
+		visitComment(tree);
 		visitField(tree, dstNs);
 		visitClass(tree, dstNs);
 

diff --git a/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java b/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java
@@ -16,6 +16,8 @@
 
 package net.fabricmc.mappingio.read;
 
+import java.nio.file.Path;
+
 import org.jetbrains.annotations.Nullable;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -25,8 +27,10 @@
 import net.fabricmc.mappingio.TestHelper;
 import net.fabricmc.mappingio.VisitOrderVerifyingVisitor;
 import net.fabricmc.mappingio.adapter.FlatAsRegularMappingVisitor;
+import net.fabricmc.mappingio.adapter.MappingSourceNsSwitch;
 import net.fabricmc.mappingio.format.MappingFormat;
 import net.fabricmc.mappingio.tree.MappingTree;
+import net.fabricmc.mappingio.tree.MappingTreeView;
 import net.fabricmc.mappingio.tree.MemoryMappingTree;
 import net.fabricmc.mappingio.tree.VisitableMappingTree;
 
@@ -138,27 +142,38 @@ public void jobfFile() throws Exception {
 		checkHoles(format);
 	}
 
-	private VisitableMappingTree checkDefault(MappingFormat format) throws Exception {
-		VisitableMappingTree tree = new MemoryMappingTree();
-		MappingReader.read(TestHelper.MappingDirs.VALID.resolve(TestHelper.getFileName(format)), format, new VisitOrderVerifyingVisitor(tree));
+	private void checkDefault(MappingFormat format) throws Exception {
+		Path path = TestHelper.MappingDirs.VALID.resolve(TestHelper.getFileName(format));
 
-		assertSubset(tree, format, testTree, null);
-		assertSubset(testTree, null, tree, format);
+		VisitableMappingTree tree = new MemoryMappingTree();
+		MappingReader.read(path, format, new VisitOrderVerifyingVisitor(tree));
+		assertEqual(format, tree, testTree);
 
-		return tree;
+		tree = new MemoryMappingTree();
+		MappingReader.read(path, format,
+				new MappingSourceNsSwitch(
+						new VisitOrderVerifyingVisitor(
+								new MappingSourceNsSwitch(
+										new VisitOrderVerifyingVisitor(tree),
+										testTree.getSrcNamespace())),
+						testTree.getDstNamespaces().get(0)));
+		assertEqual(format, tree, testTree);
 	}
 
-	private VisitableMappingTree checkHoles(MappingFormat format) throws Exception {
-		VisitableMappingTree tree = new MemoryMappingTree();
-		MappingReader.read(TestHelper.MappingDirs.VALID_WITH_HOLES.resolve(TestHelper.getFileName(format)), format, new VisitOrderVerifyingVisitor(tree));
+	private void checkHoles(MappingFormat format) throws Exception {
+		Path path = TestHelper.MappingDirs.VALID_WITH_HOLES.resolve(TestHelper.getFileName(format));
 
-		assertSubset(tree, format, testTreeWithHoles, null);
-		assertSubset(testTreeWithHoles, null, tree, format);
+		VisitableMappingTree tree = new MemoryMappingTree();
+		MappingReader.read(path, format, new VisitOrderVerifyingVisitor(tree));
+		assertEqual(format, tree, testTreeWithHoles);
+	}
 
-		return tree;
+	private void assertEqual(MappingFormat format, MappingTreeView tree, MappingTreeView referenceTree) throws Exception {
+		assertSubset(tree, format, referenceTree, null);
+		assertSubset(referenceTree, null, tree, format);
 	}
 
-	private void assertSubset(MappingTree subTree, @Nullable MappingFormat subFormat, MappingTree supTree, @Nullable MappingFormat supFormat) throws Exception {
+	private void assertSubset(MappingTreeView subTree, @Nullable MappingFormat subFormat, MappingTreeView supTree, @Nullable MappingFormat supFormat) throws Exception {
 		subTree.accept(
 				new VisitOrderVerifyingVisitor(
 						new FlatAsRegularMappingVisitor(

diff --git a/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping b/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping
@@ -3,4 +3,5 @@ CLASS class_1 class1Ns0Rename
 	METHOD method_1 method1Ns0Rename ()I
 		ARG 1 param1Ns0Rename
 	CLASS class_2 class2Ns0Rename
+		COMMENT This is a comment
 		FIELD field_2 field2Ns0Rename I
diff --git a/src/test/resources/read/valid/enigma.mappings b/src/test/resources/read/valid/enigma.mappings
@@ -3,5 +3,6 @@ CLASS class_1 class1Ns0Rename
 	METHOD method_1 method1Ns0Rename ()I
 		ARG 1 param1Ns0Rename
 	CLASS class_2 class2Ns0Rename
+		COMMENT This is a comment
 		FIELD field_2 field2Ns0Rename I
 CLASS class_3 class3Ns0Rename
diff --git a/src/test/resources/read/valid/tinyV2.tiny b/src/test/resources/read/valid/tinyV2.tiny
@@ -5,5 +5,6 @@ c	class_1	class1Ns0Rename	class1Ns1Rename
 		p	1	param_1	param1Ns0Rename	param1Ns1Rename
 		v	2	2	2	var_1	var1Ns0Rename	var1Ns1Rename
 c	class_1$class_2	class1Ns0Rename$class2Ns0Rename	class1Ns1Rename$class2Ns1Rename
+	c	This is a comment
 	f	I	field_2	field2Ns0Rename	field2Ns1Rename
 c	class_3	class3Ns0Rename	class3Ns1Rename