Hi

Here is a patch to fix the problem where commands that
are not separated by a semi-colon are not ignored.

Various possible errors are now caught instead of
being silently ignored.

In the original relation file that Gerd fixed, there was the
following:

$route=road & $network='e-road' {
  apply {
        add ref='${ref}';
        add int_ref='${int_ref}';
        add network='e-road'                # missing semi-colon
        add mkgmap:fast_road='yes';
  }
}

this was being read as:

$route=road & $network='e-road' {
  apply {
    add ref='${ref}';
        add int_ref='${int_ref}';
        add network='e-road' | 'add' | 'mkgmap:fast_road' | '=' | 'yes';
  }
}

Since 'e-road' does not contain any variables, then it was always
the value that 'network' was set to.

Now you do not need any of the semi-colons, so the original code
would be read as intended.

I don't believe that there were any more similar errors
in the style file, so this patch should have no effect for the
default style.

Of course it may find errors in custom styles, or something that did not work before may suddenly start to work as intended causing a difference.

..Steve
Index: src/uk/me/parabola/mkgmap/osmstyle/actions/ActionReader.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/uk/me/parabola/mkgmap/osmstyle/actions/ActionReader.java	(revision 4100)
+++ src/uk/me/parabola/mkgmap/osmstyle/actions/ActionReader.java	(date 1517574341000)
@@ -55,6 +55,9 @@
 			if (tok.isValue(";"))
 				continue;
 
+			if (tok.isValue("'") || tok.isValue("\""))
+				throw new SyntaxException(scanner, "quoted word found where command expected");
+
 			String cmd = tok.getValue();
 			if ("set".equals(cmd)) {
 				actions.add(readTagValue(true, changeableTags));
@@ -131,14 +134,14 @@
 	 * A name command has a number of alternatives separated by '|' characters.
 	 */
 	private Action readValueBuilder(ValueBuildedAction action) {
-		while (inActionCmd()) {
-			if (scanner.checkToken("|")) {
-				scanner.nextToken();
-				continue;
-			}
+		do {
+			if (!inActionCmd())
+				throw new SyntaxException(scanner, "unexpected end of add/set list");
+
 			String val = scanner.nextWord();
 			action.add(val);
-		}
+		} while (hasMoreWords());
+
 		usedTags.addAll(action.getUsedTags());
 		return action;
 	}
@@ -162,7 +165,9 @@
 		scanner.nextToken();
 
 		AddTagAction action = null;
-		while (inActionCmd()) {
+		do {
+			if (!inActionCmd())
+				throw new SyntaxException(scanner, "unexpected end of add/set list");
 
 			String val = scanner.nextWord();
 			if (action == null)
@@ -177,11 +182,9 @@
 			} else {
 				changeableTags.add(key + "=" + val);
 			}
-			if (scanner.checkToken("|"))
-				scanner.nextToken();
-		}
-		if (action != null)
-			usedTags.addAll(action.getUsedTags());
+		} while (hasMoreWords());
+
+		usedTags.addAll(action.getUsedTags());
 		return action;
 	}
 
@@ -199,7 +202,9 @@
 	 */
 	private AddAccessAction readAccessValue(boolean modify, Set<String> changeableTags) {
 		AddAccessAction action = null;
-		while (inActionCmd()) {
+		do {
+			if (!inActionCmd())
+				throw new SyntaxException(scanner, "unexpected end of access list");
 
 			String val = scanner.nextWord();
 			if (action == null)
@@ -210,17 +215,15 @@
 			// If the value contains a variable, then we do not know what the
 			// value will be.  Otherwise save the full tag=value
 			if (val.contains("$")) {
-				for (String accessTag : ACCESS_TAGS.keySet())
-					changeableTags.add(accessTag);
+				changeableTags.addAll(ACCESS_TAGS.keySet());
 			} else {
 				for (String accessTag : ACCESS_TAGS.keySet())
 					changeableTags.add(accessTag + "=" + val);
 			}
-			if (scanner.checkToken("|"))
-				scanner.nextToken();
-		}
-		if (action != null)
-			usedTags.addAll(action.getUsedTags());
+
+		} while (hasMoreWords());
+
+		usedTags.addAll(action.getUsedTags());
 		return action;
 	}
 	
@@ -233,6 +236,17 @@
 		return !scanner.isEndOfFile() && !scanner.checkToken("}");
 	}
 
+	private boolean hasMoreWords() {
+		if (scanner.checkToken("|")) {
+			scanner.nextToken();
+
+			if (!inActionCmd())
+				throw new SyntaxException(scanner, "unexpected end of list");
+			return true;
+		}
+		return false;
+	}
+
 	public Set<String> getUsedTags() {
 		return usedTags;
 	}
Index: src/uk/me/parabola/mkgmap/scan/TokenScanner.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/uk/me/parabola/mkgmap/scan/TokenScanner.java	(revision 4100)
+++ src/uk/me/parabola/mkgmap/scan/TokenScanner.java	(date 1517571163000)
@@ -356,7 +356,9 @@
 	}
 
 	/**
-	 * Check the value of the next token without consuming it.
+	 * Check the value of the next non-space token without consuming it.
+	 *
+	 * Any white space will be consumed
 	 *
 	 * @param val String value to compare against.
 	 * @return True if the next token has the same value as the argument.
Index: test/uk/me/parabola/mkgmap/osmstyle/ActionReaderTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- test/uk/me/parabola/mkgmap/osmstyle/ActionReaderTest.java	(revision 4100)
+++ test/uk/me/parabola/mkgmap/osmstyle/ActionReaderTest.java	(date 1517574463000)
@@ -32,6 +32,7 @@
 import uk.me.parabola.mkgmap.scan.SyntaxException;
 import uk.me.parabola.mkgmap.scan.TokenScanner;
 
+import org.hamcrest.core.StringContains;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
@@ -255,6 +256,47 @@
 		assertEquals("second alternative", "default value", el.getTag("fred"));
 	}
 
+	@Test
+	public void testMultipleNoSeparators() {
+		List<Action> actions = readActionsFromString("{" +
+				"set park='${notset}' | yes " +
+				"add fred=other " +
+				"set pooh=bear}");
+
+		assertEquals("number of actions", 3, actions.size());
+
+		Element el = stdElementRun(actions);
+
+		assertEquals("park set to yes", "yes", el.getTag("park"));
+		assertEquals("fred set", "other", el.getTag("fred"));
+		assertEquals("pooh set", "bear", el.getTag("pooh"));
+	}
+
+	@Test(expected = SyntaxException.class)
+	public void testErrorShortSet() {
+		readActionsFromString("{set park= }");
+	}
+
+	@Test(expected = SyntaxException.class)
+	public void testMangledSet() {
+		readActionsFromString("{set park=yes some other junk }");
+	}
+
+	@Test(expected = SyntaxException.class)
+	public void testErrorMangledList() {
+		readActionsFromString("{set park='${notset}' | }");
+	}
+
+	@Test
+	public void testErrorExtraQuotedWord() {
+		try {
+			readActionsFromString("{set park=yes 'some' other junk }");
+			assert false;  // should not get here
+		} catch (SyntaxException e) {
+			assertThat(e.getMessage(), new StringContains("quoted word found where command expected"));
+		}
+	}
+
 	private Element stdElementRun(List<Action> actions) {
 		Rule rule = new ActionRule(null, actions);
 		Element el = makeElement();
_______________________________________________
mkgmap-dev mailing list
mkgmap-dev@lists.mkgmap.org.uk
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev

Reply via email to