Bug 445177 - Fix an over-eager optimization that can discard a desired

alternative in TemplateIdStrategy

Change-Id: Ia82460e5e3a3420cbb13f7abd698f0173336b123
Signed-off-by: Nathan Ridge <zeratul976@hotmail.com>

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TemplateTests.java

@@ -6086,6 +6086,13 @@ public class AST2TemplateTests extends AST2TestBase {
 	public void testTemplateAmbiguityInDeleteExpression_364225() throws Exception {
 		parseAndCheckBindings();
 	}
+	
+	//	template<int, int> struct a {};
+	//	const int b = 0, c = 1;
+	//	int a<b<c,b<c>::*mp6;  // syntax error here
+	public void testTemplateIDAmbiguity_445177() throws Exception {
+		parseAndCheckBindings();
+	}
 
 	//	template <typename T> void foo(T);
 	//	template <typename T> void foo(T, typename T::type* = 0);

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/AbstractGNUSourceCodeParser.java

@@ -99,6 +99,7 @@ import org.eclipse.cdt.internal.core.parser.scanner.ILocationResolver;
  * Base class for the c- and c++ parser.
  */
 public abstract class AbstractGNUSourceCodeParser implements ISourceCodeParser {
+	// see TemplateIdStrategy for an explanation of what this does
 	public interface ITemplateIdStrategy {
 		boolean shallParseAsTemplateID(IASTName name);
 	}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/GNUCPPSourceParser.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2002, 2014 IBM Corporation and others.
+ * Copyright (c) 2002, 2015 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,6 +15,7 @@
  *     Sergey Prigogin (Google)
  *     Thomas Corbat (IFS)
  *     Anders Dahlberg (Ericsson) - bug 84144
+ *     Nathan Ridge
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.dom.parser.cpp;
 
@@ -261,7 +262,7 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
     		try {
     			return nameSpecifier(ctx, strat);
     		} catch (BacktrackException e) {
-    			if (strat.setNextAlternative()) {
+    			if (strat.setNextAlternative(true /* previous alternative failed to parse */)) {
     				backup(m);
     			} else {
     				throw e;
@@ -342,14 +343,14 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
 				final int haveArgs = haveTemplateArguments(inBinaryExpression);
             	boolean templateID= true;
             	if (!keywordTemplate) {
-					if (haveArgs == -1) {
+					if (haveArgs == NO_TEMPLATE_ID) {
 						templateID= false;
-					} else if (haveArgs == 0) {
+					} else if (haveArgs == AMBIGUOUS_TEMPLATE_ID) {
 						templateID= strat.shallParseAsTemplateID(name);
 					}
             	}
             	if (templateID) {
-            		if (haveArgs == -1)
+            		if (haveArgs == NO_TEMPLATE_ID)
             			throwBacktrack(LA(1));
 
             		nameSpec= (ICPPASTName) addTemplateArguments(name, strat);
@@ -560,7 +561,7 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
         			break;
         		}
         	}
-        	return 0;
+        	return AMBIGUOUS_TEMPLATE_ID;
         } finally {
         	backup(mark);
         }
@@ -1156,11 +1157,11 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
 				if (variants != null) {
 					variants = new Variant(variants, e, strat.getTemplateNames(), lastToken.getOffset());
 				}
-				if (!strat.setNextAlternative()) {
+				if (!strat.setNextAlternative(false /* previous alternative parsed ok */)) {
 					break;
 				}
 			} catch (BacktrackException e) {
-				if (!strat.setNextAlternative()) {
+				if (!strat.setNextAlternative(true /* previous alternative failed to parse */)) {
 					if (lastToken == null)
 						throw e;
 
@@ -1839,6 +1840,11 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
         case IToken.tBITCOMPLEMENT:
         case IToken.t_decltype: {
             IASTName name = qualifiedName(ctx, strat);
+            // A qualified-name's last name can sometimes be empty in a declaration (e.g. in "int A::*x",
+            // "A::" is a valid qualified-name with an empty last name), but not in an expression
+            // (unless we are invoking code completion at the '::').
+            if (name.getLookupKey().length == 0 && LT(1) != IToken.tEOC)
+                throwBacktrack(LA(1));
             IASTIdExpression idExpression = nodeFactory.newIdExpression(name);
             return setRange(idExpression, name);
         }
@@ -2228,7 +2234,7 @@ public class GNUCPPSourceParser extends AbstractGNUSourceCodeParser {
     			try {
     				return templateParameterList(result);
     			} catch (BacktrackException e) {
-    				if (!fTemplateParameterListStrategy.setNextAlternative()) {
+    				if (!fTemplateParameterListStrategy.setNextAlternative(true /* previous alternative failed to parse */)) {
     					fTemplateParameterListStrategy= null;
     					throw e;
     				}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/TemplateIdStrategy.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011 Wind River Systems, Inc. and others.
+ * Copyright (c) 2011, 2015 Wind River Systems, Inc. and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -7,6 +7,7 @@
  *
  * Contributors:
  *    Markus Schorn - initial API and implementation
+ *    Nathan Ridge  - added comments and fixed bug 445177
  *******************************************************************************/ 
 
 package org.eclipse.cdt.internal.core.dom.parser.cpp;
@@ -20,23 +21,53 @@ import org.eclipse.cdt.core.parser.util.ArrayUtil;
 import org.eclipse.cdt.internal.core.dom.parser.AbstractGNUSourceCodeParser.ITemplateIdStrategy;
 
 /**
- * Governs backtracking through multiple variants due to the ambiguous meaning of '<'.
+ * This class is used to track alternatives for parsing segments of code that involve '<' tokens.
- * @see NameOrTemplateIDVariants
+ * 
+ * The '<' token can be either a less-than operator or part of a template-id.
+ * When parsing, we potentially need to consider both possibilities for each use of '<'.
+ * 
+ * An instance of this class is used to track alternative parses in a segment of code that includes one or
+ * more uses of '<' preceded by names. An alternative consists of a choices (template-id or not) for each
+ * name. At a given point in time, the instance has a notion of a current alternative, and a current
+ * position within that alternative. 
+ * 
+ * @see also NameOrTemplateIDVariants, which is used together with this class to deal with ambiguities
+ * involving '<' when parsing in an expression context.
  */
 final class TemplateIdStrategy implements ITemplateIdStrategy {
+	// The current position in the current alternative. 
+	// The next call to shallParseAsTemplateID() will return whether in the current alternative,
+	// the name at index (fCurrentBranchPoint + 1) should be parsed as a template-id.
 	private int fCurrentBranchPoint;
+	
+	// The current alternative, represented as a bitset with one bit for each name.
+	// A bit corresponding to a name is clear if the name should be parsed as a template-id, and set if
+	// it should not.
+	// For the first alternative, this bitset is null, and this is interpreted as all zeros (i.e. every name
+	// is parsed as a template-id).
 	private BitSet fSimpleIDs;
+	
+	// The set of names which are parsed as template-ids in the current alternative.
 	private IASTName[] fTemplateNames;
 	
 	public TemplateIdStrategy() {
 		fCurrentBranchPoint= -1;
 		fTemplateNames= IASTName.EMPTY_NAME_ARRAY;
 	}
-
+	
+	/**
+	 * Returns whether 'name' should be parsed as a template-id according to the current alternative.
+	 * For each alternative, this is expected to be called once for each name in a segment code for which
+	 * this choice is ambiguous. 
+	 * For the first alternative, these calls are used to initially populate the list of names.
+	 * For subsequent alternatives, these calls are expected for the same names in the same order.
+	 */
 	@Override
 	public boolean shallParseAsTemplateID(IASTName name) {
 		fCurrentBranchPoint++;
 		
+		// 'fSimpleIDs == null' means we're on the first alternative. 
+		// On the first alternative, everything is parsed as a template-id.
 		boolean templateID= fSimpleIDs == null || !fSimpleIDs.get(fCurrentBranchPoint);
 		if (templateID) {
 			fTemplateNames= ArrayUtil.append(fTemplateNames, name);
@@ -44,23 +75,52 @@ final class TemplateIdStrategy implements ITemplateIdStrategy {
 		return templateID;
 	}
 
-	public boolean setNextAlternative() {
+	/**
-		int bp = fCurrentBranchPoint;
+	 * Advance to the next alternative parse, or return false is there is none.
-		if (bp < 0)
+	 * After a call to this, the current position is reset to the beginning of the (next) alternative.
+	 * @param previousAlternativeFailedToParse whether this function is being called because the previous
+	 *                                         alternative failed to parse
+	 */
+	public boolean setNextAlternative(boolean previousAlternativeFailedToParse) {
+		// No one has called shallParseAsTemplateID() for the current alternative, so there are no
+		// ambiguous names. Therefore, there are no more alternatives.
+		if (fCurrentBranchPoint < 0) {
 			return false;
+		}
 		
+		// Reset the current position, saving the old one, which should point to the last name in the
+		// bitset for which parsing was attempted during the previous alternative.
+		int bp = fCurrentBranchPoint;
 		fCurrentBranchPoint= -1;
+		
+		// Reset the list of names that were parsed as template-ids, saving the list for the previous
+		// alternative. 
 		IASTName[] names = getTemplateNames();
+		// Note that 'names' here contains the list of names for which there is a '0' in the bitset.
 		int nameLen= names.length;
 		fTemplateNames= IASTName.EMPTY_NAME_ARRAY;
+		
+		// If the previous alternative was the first, the bitset is still null. Create it.
 		if (fSimpleIDs == null) {
 			fSimpleIDs= new BitSet();
 		}
 
-		// Set a new branch as far right as possible.
+		// Advance to the next alternative by finding the right-most '0' in the bitset, and setting it to '1',
+		// and bits to the right of it to '0'. In this way, successive calls to this function will iterate
+		// over all possible alternatives.
+		// Note that in searching for the right-most '0', we start at 'bp', not the last element of the 
+		// bitset. The reason is that if 'bp' is not the last element of the bitset, it means that during the
+		// previous alternative, we failed the parse before getting beyond 'bp'. This means that there is a
+		// problem with one of the choices up to and including 'bp', so there's no point in trying another
+		// alternatives that keeps these choices the same.
 		while (bp >= 0) {
 			if (!fSimpleIDs.get(bp)) {
-				if (nameLen == 0 || !hasMultipleArgs(names[--nameLen])) {
+				// Optimization (bug 363609): if during the previous alternative, a name was parsed as a
+				// template-id with multiple template arguments, it's not going to be parsed differently in
+				// a subsequent alternative, so keep it as a template-id.
+				// Of course, this optimization is only sound if the previous alternative was parsed
+				// successfully (bug 445177).
+				if (previousAlternativeFailedToParse || nameLen == 0 || !hasMultipleArgs(names[--nameLen])) {
 					fSimpleIDs.clear(bp+1, Integer.MAX_VALUE);
 					fSimpleIDs.set(bp);
 					return true;
@@ -68,6 +128,8 @@ final class TemplateIdStrategy implements ITemplateIdStrategy {
 			}
 			bp--;
 		}
+		
+		// The bitset was all ones - there are no more alternatives.
 		return false;
 	}