clean up of findGuardForGroup in HoistingProcessor

removed special case for non-trivial cardinality - use regular
findGuardForElement instead

adjusted test cases to follow antlr semantic - ignore following
predicates

made HoistingProcessor Singleton

preparation for better caching

org.eclipse.xtext.tests/src/org/eclipse/xtext/xtext/generator/hoisting/HoistingProcessorTest.java

@@ -288,7 +288,7 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
 		assertFalse(guard.isTrivial());
 		assertTrue(guard.hasTerminal());
-		assertEquals("((" + getSyntaxForKeywordToken("s", 1) + " || (p1)) && (" + getSyntaxForKeywordToken("a", 1) + " || (p0)))", guard.render());
+		assertEquals("(" + getSyntaxForKeywordToken("a", 1) + " || (p0))", guard.render());
 	}
 	
 	@Test
@@ -344,7 +344,7 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
 		assertFalse(guard.isTrivial());
 		assertTrue(guard.hasTerminal());
-		assertEquals("((" + getSyntaxForKeywordToken("s", 1) + " || (p1)) && (" + getSyntaxForKeywordToken("a", 1) + " || (p0)))", guard.render());
+		assertEquals("(" + getSyntaxForKeywordToken("a", 1) + " || (p0))", guard.render());
 	}
 	
 	@Test
@@ -443,6 +443,7 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		// @formatter:off
 		String model =
 			MODEL_PREAMBLE +
+			"hoistingDebug\n" +
 			"S: ($$ p0 $$?=> 'a')? & ($$ p1 $$?=> 'b');";
 		// @formatter:off
 		XtextResource resource = getResourceFromString(model);

org.eclipse.xtext.xtext.generator/src/org/eclipse/xtext/xtext/generator/parser/antlr/hoisting/HoistingProcessor.java

@@ -63,12 +63,14 @@ import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.token.Token;
 import static org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.utils.DebugUtils.*;
 import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.utils.StreamUtils;
 
+import com.google.inject.Singleton;
+
 /**
  * @author overflow - Initial contribution and API
  */
+@Singleton
 public class HoistingProcessor {
-	private Map<String, HoistingGuard> ruleCache = new HashMap<>();
+	private Map<String, HoistingGuard> elementCache = new HashMap<>();
-	private Map<Group, HoistingGuard> groupCache = new HashMap<>();
 	
 	private Logger log = Logger.getLogger(this.getClass());
 	
@@ -115,7 +117,7 @@ public class HoistingProcessor {
 				)
 			);
 		} catch(TokenAnalysisAbortedException e) {
-			throw new TokenAnalysisAbortedException(e.getMessage(), e, currentRule);
+			throw new TokenAnalysisAbortedException(e, currentRule);
 		}
 	}
 
@@ -343,87 +345,19 @@ public class HoistingProcessor {
 	}
 	
 	private HoistingGuard findGuardForGroup(Group group, AbstractRule currentRule) {
-		log.info("find guard for group");
-		
 		GroupGuard groupGuard = new GroupGuard();
 		
-		Iterator<AbstractElement> iterator = group.getElements().iterator();
+		for(AbstractElement element : group.getElements()) {
-		while (iterator.hasNext()) {
+			HoistingGuard guard = findGuardForElement(element, currentRule);
-			AbstractElement element = iterator.next();
+			groupGuard.add(guard);
 			
-			String cardinality = element.getCardinality();
+			// if cardinality is + and there is a terminal we don't need to consider
-			if (isTrivialCardinality(element) || isOneOrMoreCardinality(element)) {
+			// the following elements
-				
+			// if cardinality is + and there is no token the guard won't change even if the
-				HoistingGuard guard = findGuardForElementWithTrivialCardinality(element, currentRule);
+			// element could be repeated
-				groupGuard.add(guard);
+			if (guard.hasTerminal()) {
-				
+				groupGuard.setHasTerminal();
-				// if cardinality is + and there is a terminal we don't need to consider
+				break;
-				// the following elements
-				// if cardinality is + and there is no token the guard won't change even if the
-				// element could be repeated
-				if (guard.hasTerminal()) {
-					groupGuard.setHasTerminal();
-					break;
-				}
-			} else if (isOptionalCardinality(element)) {
-				// though not technically necessary, rewrite tree to context checks are not needed 
-				
-				// rewrite cardinality to alternatives
-				// A? B -> A B | B
-				// A* B -> A+ B | B -> A B | B (see above)
-				
-				// make copy of every element because we can't use any element twice
-				List<AbstractElement> remainingElementsInGroup = StreamUtils.fromIterator(iterator)
-						.collect(Collectors.toList());
-				
-				if (remainingElementsInGroup.isEmpty()) {
-					// B is empty
-					// context will be taken from parent element
-					
-					HoistingGuard guard = findGuardForOptionalCardinalityWithoutContext(element, currentRule);
-					groupGuard.add(guard);
-					
-					if (guard.hasTerminal()) {
-						groupGuard.setHasTerminal();
-					}
-					
-					// there are no following elements
-					break;
-				} else {
-					// B is non-empty
-					// -> construct context for alternatives
-					
-					// we need a clone of the element because we need to set the cardinality without changing the
-					// original syntax tree
-					AbstractElement clonedElement = copy(element);
-					clonedElement.setCardinality(null);
-					
-					// make copy of first branch and add the cloned element
-					List<AbstractElement> remainingElementsInGroupIncludingCurrent = new LinkedList<>(remainingElementsInGroup);
-					remainingElementsInGroupIncludingCurrent.add(0, clonedElement);
-							
-					Group virtualPathRemaining = XtextFactory.eINSTANCE.createGroup();
-					setElementsOfCompoundElement(virtualPathRemaining, remainingElementsInGroup);
-					
-					Group virtualPathRemainingPlusCurrent = XtextFactory.eINSTANCE.createGroup();
-					setElementsOfCompoundElement(virtualPathRemainingPlusCurrent, remainingElementsInGroupIncludingCurrent);
-					
-					Alternatives virtualAlternatives = XtextFactory.eINSTANCE.createAlternatives();
-					setElementsOfCompoundElement(virtualAlternatives, Arrays.asList(virtualPathRemaining, virtualPathRemainingPlusCurrent));
-					
-					// get Guard for virtual alternatives
-					HoistingGuard guard = findGuardForElementWithTrivialCardinality(virtualAlternatives, currentRule);
-					groupGuard.add(guard);
-					
-					if (guard.hasTerminal()) {
-						groupGuard.setHasTerminal();
-					}
-
-					// following elements are included in alternative, no need to check further
-					break;
-				}
-			} else {
-				throw new IllegalArgumentException("unknown cardinality: " + cardinality);
 			}
 		}
 		
@@ -433,13 +367,7 @@ public class HoistingProcessor {
 	// TODO: make private
 	public HoistingGuard findGuardForRule(AbstractRule rule) {
 		log.info("finding guard for rule: " + rule.getName());
-		HoistingGuard guard = ruleCache.get(rule.getName());
+		return findGuardForElement(rule.getAlternatives(), rule);
-		if (guard == null) {
-			log.info("not in cache");
-			guard = findGuardForElement(rule.getAlternatives(), rule);
-			ruleCache.put(rule.getName(), guard);
-		}
-		return guard;
 	}
 	
 	private boolean pathHasTokenOrAction(AbstractElement path) {
@@ -520,15 +448,26 @@ public class HoistingProcessor {
 	}
 	
 	private HoistingGuard findGuardForElement(AbstractElement element, AbstractRule currentRule) {
+		String path = getPathOfElement(element);
+		
+		HoistingGuard guard = null;//elementCache.get(path);
+		if (guard != null) {
+			log.info("from cache: " + path);
+			return guard;
+		}
+		
 		if (isTrivialCardinality(element) ||
 			isOneOrMoreCardinality(element)
 		) {
-			return findGuardForElementWithTrivialCardinality(element, currentRule);
+			guard = findGuardForElementWithTrivialCardinality(element, currentRule);
 		} else if (isOptionalCardinality(element)) {
-			return findGuardForOptionalCardinalityWithoutContext(element, currentRule);
+			guard = findGuardForOptionalCardinalityWithoutContext(element, currentRule);
 		} else {
 			throw new IllegalArgumentException("unknown cardinality: " + element.getCardinality());
 		}
+		
+		elementCache.put(path, guard);
+		return guard;
 	}
 	
 	private HoistingGuard findGuardForElementWithTrivialCardinality(AbstractElement element, AbstractRule currentRule) {
@@ -538,12 +477,7 @@ public class HoistingProcessor {
 		if (element instanceof Alternatives) {
 			return findGuardForAlternatives((Alternatives) element, currentRule);
 		} else if (element instanceof Group) {
-			HoistingGuard guard = groupCache.get(element);
+			return findGuardForGroup((Group) element, currentRule);
-			if (guard == null) {
-				guard = findGuardForGroup((Group) element, currentRule);
-				groupCache.put((Group) element, guard);
-			}
-			return guard;
 		} else if (element instanceof AbstractSemanticPredicate) {
 			return new PredicateGuard((AbstractSemanticPredicate) element);
 		} else if (Token.isToken(element)) {