added empty/non-empty test into nfa context ambig check; was missing. added comments

[git-p4: depot-paths = "//depot/code/antlr4/main/": change = 6810]

tool/src/org/antlr/v4/analysis/NFAContext.java

@@ -115,32 +115,6 @@ public class NFAContext {
 		return true;
 	}
 
-	/** Two contexts conflict() if they are equals() or one is a stack suffix
-	 *  of the other.  For example, contexts [21 12 $] and [21 9 $] do not
-	 *  conflict, but [21 $] and [21 12 $] do conflict.  Note that I should
-	 *  probably not show the $ in this case.  There is a dummy node for each
-	 *  stack that just means empty; $ is a marker that's all.
-	 *
-	 *  This is used in relation to checking conflicts associated with a
-	 *  single NFA state's configurations within a single DFA state.
-	 *  If there are configurations s and t within a DFA state such that
-	 *  s.state=t.state && s.alt != t.alt && s.ctx conflicts t.ctx then
-	 *  the DFA state predicts more than a single alt--grammar is ambiguous.
-	 *  Two contexts conflict if they are the same or if one is a suffix
-	 *  of the other.
-	 *
-	 *  When comparing contexts, if one context has a stack and the other
-	 *  does not then they should be considered the same context.  The only
-	 *  way for an NFA state p to have an empty context and a nonempty context
-	 *  is the case when closure falls off end of rule without a call stack
-	 *  and re-enters the rule with a context.  This resolves the issue I
-	 *  discussed with Sriram Srinivasan Feb 28, 2005 about not terminating
-	 *  fast enough upon nondeterminism.
-	 */
-	public boolean conflictsWith(NFAContext other) {
-		return this.suffix(other) || this.equals(other);
-	}
-
 	/** [$] suffix any context
 	 *  [21 $] suffix [21 12 $]
 	 *  [21 12 $] suffix [21 $]

tool/src/org/antlr/v4/analysis/PredictionDFAFactory.java

@@ -292,23 +292,23 @@ public class PredictionDFAFactory {
 	 *  then clearly the exact same computation is proposed.  If a context
 	 *  is a suffix of the other, then again the computation is in an
 	 *  identical context.  beta $ and beta alpha $ are considered the same stack.
-	 *  We could walk configurations linearly doing the comparison instead
-	 *  of a set for exact matches but it's much slower because you can't
-	 *  do a Set lookup.  I use exact match as ANTLR
-	 *  always detect the conflict later when checking for context suffixes...
-	 *  I check for left-recursive stuff and terminate before analysis to
-	 *  avoid need to do this more expensive computation.
+	 *  We could walk configurations linearly doing suuch a comparison instead
+	 *  of a set lookup for exact matches but it's much slower because you can't
+	 *  do a Set lookup.  I use exact match as ANTLR always detect the conflict
+	 *  later when checking for ambiguous configs (it tests context suffixes).
+	 *
+	 *  TODO: change comment once I figure out if we can ignore suffixes in favor of empty/non test only
 	 *
 	 *  Side-effect warning:
 	 *
 	 *  Rather than pass in a list of configs to update or return and
 	 *  collect lots of little config lists, it's more efficient to
 	 *  modify d's config list directly.
+	 *
+	 *  Rather than pass closureBusy everywhere, I use a field of this object.
 	 */
 	public void closure(DFAState d, NFAConfig c, boolean collectPredicates) {
-		//NFAConfig proposedNFAConfig = new NFAConfig(s, altNum, context, semanticContext);
-
-		if ( closureBusy.contains(c) ) return;
+		if ( closureBusy.contains(c) ) return; // don't re-attempt same closure(c)
 		closureBusy.add(c);
 
 		// p itself is always in closure

tool/src/org/antlr/v4/analysis/Resolver.java

@@ -23,8 +23,8 @@ public class Resolver {
 	}
 	
 	/** Walk each NFA configuration in this DFA state looking for a conflict
-	 *  where (s|i|ctx) and (s|j|ctx) exist, indicating that state s with
-	 *  conflicting ctx predicts alts i and j.  Return an Integer set
+	 *  where (s|i|ctx1) and (s|j|ctx2) exist such that ctx1 and ctx2 conflict.
+	 *  That indicates that state s predicts alts i and j.  Return an Integer set
 	 *  of the alternative numbers that conflict.  Two contexts conflict if
 	 *  they are equal or one is a stack suffix of the other or one is
 	 *  the empty context.  The conflict is a true ambiguity. No amount
@@ -35,6 +35,13 @@ public class Resolver {
 	 *  there is more than one configuration.  The configurations' predicted
 	 *  alt must be different or must have different contexts to avoid a
 	 *  conflict.
+	 *
+	 *  We check exact config match only in closure-busy test so we'll see
+	 *  empty and nonempty contexts here for same state and alt; e.g.,
+	 *  (s|i|$) and (s|i|[21 3 $]).
+	 *
+	 *  TODO: suffix degenerates to one empty one nonempty; avoid some tests?
+	 *  TODO: or perhaps check if i, j are already in and don't do compare?
 	 */
 	public Set<Integer> getAmbiguousAlts(DFAState d) {
 		//System.out.println("getNondetAlts for DFA state "+stateNumber);
@@ -100,8 +107,12 @@ public class Resolver {
 					// conflicts means s.ctx==t.ctx or s.ctx is a stack
 					// suffix of t.ctx or vice versa (if alts differ).
 					// Also a conflict if s.ctx or t.ctx is empty
+					// TODO: might be faster to avoid suffix test if i or j already in ambiguousAlts
+					// that set is usually 2 alts, maybe three.  Use a List.
 					boolean altConflict = s.alt != t.alt;
-					boolean ctxConflict = s.context.conflictsWith(t.context);
+					boolean ctxConflict = (s.context==null && t.context!=null) ||
+										  (s.context!=null && t.context==null) ||
+										  (s.context.suffix(t.context) || s.context.equals(t.context));
 					if ( altConflict && ctxConflict ) {
 						//System.out.println("ctx conflict between "+s+" and "+t);
 						ambiguousAlts.add(s.alt);