Improved heuristic header substitution algorithm and unified it between

Add Include and Organize Includes commands.

Change-Id: I0d4a1110a8b89ca49d55eb82eddb29e7d8bcd548

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/HeaderSubstitutor.java

@@ -201,9 +201,12 @@ public class HeaderSubstitutor {
 		String symbolName = request.getBinding().getName();
 		ArrayDeque<IIndexFile> front = new ArrayDeque<>();
 		HashSet<IIndexFile> processed = new HashSet<>();
+		IIndexFile bestCandidate = null;
+		IIndexFile candidateWithoutExtension = null;
+		IIndexFile candidateWithMatchingName = null;
 
 		try {
-			// Look for headers without an extension and a matching name.
+			// Look for headers matching by name and headers without an extension.
 			if (fContext.isCXXLanguage()) {
 				front.addAll(indexFiles);
 				processed.addAll(indexFiles);
@@ -213,10 +216,18 @@ public class HeaderSubstitutor {
 
 					String path = IncludeUtil.getPath(file);
 
-					if (!hasExtension(path) && getFilename(path).equalsIgnoreCase(symbolName)) {
+					if (getFilename(path).equalsIgnoreCase(symbolName)) {
+						if (!hasExtension(path)) {
 							// A C++ header without an extension and with a name which matches the name
-						// of the symbol which should be declared is a perfect candidate for inclusion.
+							// of the symbol that should be declared is a perfect candidate for inclusion.
-						return IndexLocationFactory.getAbsolutePath(file.getLocation());
+							bestCandidate = file;
+							break;
+						}
+						if (candidateWithMatchingName == null)
+							candidateWithMatchingName = file;
+					} else if (!hasExtension(path)) {
+						if (candidateWithoutExtension == null)
+							candidateWithoutExtension = file;
 					}
 
 					// Process the next level of the include hierarchy.
@@ -231,7 +242,15 @@ public class HeaderSubstitutor {
 				}
 			}
 
-			// Repeat the process, this time only looking for headers without an extension.
+			if (bestCandidate == null) {
+				bestCandidate = candidateWithoutExtension;
+			}
+			if (bestCandidate == null) {
+				bestCandidate = candidateWithMatchingName;
+			}
+
+			if (bestCandidate == null) {
+				// Repeat inclusion tree search, this time looking for any header included by a source file.
 				front.clear();
 				front.addAll(indexFiles);
 				processed.clear();
@@ -240,13 +259,6 @@ public class HeaderSubstitutor {
 				while (!front.isEmpty()) {
 					IIndexFile file = front.remove();
 
-				String path = IncludeUtil.getPath(file);
-
-				if (fContext.isCXXLanguage() && !hasExtension(path)) {
-					// A C++ header without an extension is still a very good candidate for inclusion.
-					return IndexLocationFactory.getAbsolutePath(file.getLocation());
-				}
-
 					// Process the next level of the include hierarchy.
 					IIndexInclude[] includes = fContext.getIndex().findIncludedBy(file, 0);
 					for (IIndexInclude include : includes) {
@@ -254,13 +266,17 @@ public class HeaderSubstitutor {
 						if (!processed.contains(includer)) {
 							URI uri = includer.getLocation().getURI();
 							if (IncludeUtil.isSource(includer, fContext.getProject()) || isWorkspaceFile(uri)) {
-							return IndexLocationFactory.getAbsolutePath(file.getLocation());
+								bestCandidate = file;
+								break;
 							}
 							front.add(includer);
 							processed.add(includer);
 						}
 					}
 				}
+			}
+			if (bestCandidate != null)
+				return IndexLocationFactory.getAbsolutePath(bestCandidate.getLocation());
 		} catch (CoreException e) {
 			CUIPlugin.log(e);
 		}

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreationContext.java

@@ -33,7 +33,7 @@ import org.eclipse.cdt.internal.corext.codemanipulation.InclusionContext;
 /**
  * Context for managing include statements.
  */
-public class IncludeCreationContext extends InclusionContext {
+public final class IncludeCreationContext extends InclusionContext {
 	private final IIndex fIndex;
 	private final Set<IPath> fHeadersToInclude;
 	private final Set<IPath> fHeadersAlreadyIncluded;
@@ -164,4 +164,13 @@ public class IncludeCreationContext extends InclusionContext {
 		}
 		return null;
 	}
+
+	/**
+	 * Checks if the given file is suitable for inclusion. A file is suitable for inclusion if it is a header
+	 * file, or if it is already included by some other file.
+	 */
+	public final boolean canBeIncluded(IIndexFile indexFile) throws CoreException {
+		return !IncludeUtil.isSource(indexFile, getProject()) ||
+				fIndex.findIncludedBy(indexFile, 0).length != 0;
+	}
 }

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreator.java

@@ -15,23 +15,18 @@ package org.eclipse.cdt.internal.ui.refactoring.includes;
 
 import static org.eclipse.cdt.core.index.IndexLocationFactory.getAbsolutePath;
 
-import java.net.URI;
-import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.NullProgressMonitor;
-import org.eclipse.core.runtime.content.IContentType;
 import org.eclipse.jface.text.IRegion;
 import org.eclipse.jface.text.ITextSelection;
 import org.eclipse.text.edits.InsertEdit;
@@ -39,7 +34,6 @@ import org.eclipse.text.edits.MultiTextEdit;
 
 import com.ibm.icu.text.Collator;
 
-import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.dom.IName;
 import org.eclipse.cdt.core.dom.ast.DOMException;
 import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
@@ -72,7 +66,6 @@ import org.eclipse.cdt.core.dom.ast.cpp.ICPPVariable;
 import org.eclipse.cdt.core.index.IIndex;
 import org.eclipse.cdt.core.index.IIndexBinding;
 import org.eclipse.cdt.core.index.IIndexFile;
-import org.eclipse.cdt.core.index.IIndexInclude;
 import org.eclipse.cdt.core.index.IIndexMacro;
 import org.eclipse.cdt.core.index.IIndexName;
 import org.eclipse.cdt.core.index.IndexFilter;
@@ -90,7 +83,6 @@ import org.eclipse.cdt.internal.core.dom.rewrite.commenthandler.ASTCommenter;
 import org.eclipse.cdt.internal.core.dom.rewrite.commenthandler.NodeCommentMap;
 import org.eclipse.cdt.internal.core.dom.rewrite.util.ASTNodes;
 import org.eclipse.cdt.internal.core.model.ASTStringUtil;
-import org.eclipse.cdt.internal.core.resources.ResourceLookup;
 import org.eclipse.cdt.internal.core.util.TextUtil;
 import org.eclipse.cdt.internal.corext.codemanipulation.IncludeInfo;
 import org.eclipse.cdt.internal.corext.codemanipulation.StyledInclude;
@@ -168,14 +160,15 @@ public class IncludeCreator {
 				}
 				IIndexName[] definitions= null;
 				// class, struct, union, enum-type, enum-item
-				if (indexBinding instanceof ICompositeType || indexBinding instanceof IEnumeration || indexBinding instanceof IEnumerator) {
+				if (indexBinding instanceof ICompositeType || indexBinding instanceof IEnumeration
+						|| indexBinding instanceof IEnumerator) {
 					definitions= index.findDefinitions(indexBinding);
-				} else if (indexBinding instanceof ITypedef || (indexBinding instanceof IFunction)) {
+				} else if (indexBinding instanceof ITypedef || indexBinding instanceof IFunction) {
 					definitions = index.findDeclarations(indexBinding);
 				}
 				if (definitions != null) {
 					for (IIndexName definition : definitions) {
-						considerForInclusion(definition, indexBinding, index, headerSubstitutor,
+						considerForInclusion(ast, definition, indexBinding, index, headerSubstitutor,
 								candidatesMap);
 					}
 					if (definitions.length > 0 && adaptedBinding != null)
@@ -185,7 +178,7 @@ public class IncludeCreator {
 			IIndexMacro[] macros = index.findMacros(nameChars, filter, new NullProgressMonitor());
 			for (IIndexMacro macro : macros) {
 				IIndexName definition = macro.getDefinition();
-				considerForInclusion(definition, macro, index, headerSubstitutor, candidatesMap);
+				considerForInclusion(ast, definition, macro, index, headerSubstitutor, candidatesMap);
 			}
 
 			final ArrayList<IncludeCandidate> candidates = new ArrayList<>(candidatesMap.values());
@@ -475,27 +468,30 @@ public class IncludeCreator {
 	}
 
 	/**
-	 * Adds an include candidate to the <code>candidates</code> map if the file containing
+	 * Adds an include candidate to the {@code candidates} map if the file containing the definition
-	 * the definition is suitable for inclusion.
+     * is suitable for inclusion.
 	 */
-	private void considerForInclusion(IIndexName definition, IIndexBinding binding, IIndex index,
+	private void considerForInclusion(IASTTranslationUnit ast, IIndexName definition, IIndexBinding binding,
-			HeaderSubstitutor headerSubstitutor, Map<String, IncludeCandidate> candidates) throws CoreException {
+			IIndex index, HeaderSubstitutor headerSubstitutor, Map<String, IncludeCandidate> candidates)
+			throws CoreException {
 		if (definition == null) {
 			return;
 		}
 		IIndexFile file = definition.getFile();
 		// Consider the file for inclusion only if it is not a source file,
 		// or a source file that was already included by some other file.
-		if (!isSource(getPath(file)) || index.findIncludedBy(file, 0).length > 0) {
+		if (fContext.canBeIncluded(file)) {
 			IncludeInfo include;
-			if (fContext.getPreferences().heuristicHeaderSubstitution) {
-				include = getIncludeByHeuristic(file, index);
-			} else {
 			IPath header = getAbsolutePath(file.getLocation());
 			header = headerSubstitutor.getPreferredRepresentativeHeader(header);
+			if (fContext.getPreferences().heuristicHeaderSubstitution) {
+				boolean reachable = ast.getIndexFileSet().contains(file);
+				InclusionRequest request =
+						new InclusionRequest(binding, Collections.singletonMap(file, header), reachable);
+				header = headerSubstitutor.getPreferredRepresentativeHeaderByHeuristic(request);
+			}
 			IncludeGroupStyle style = fContext.getIncludeStyle(header);
 			include = fContext.createIncludeInfo(header, style);
-			}
 
 			if (include != null) {
 				IncludeCandidate candidate = new IncludeCandidate(binding, include);
@@ -611,7 +607,6 @@ public class IncludeCreator {
 					} else {
 						break;
 					}
-					
 				} catch (DOMException e) {
 					break;
 				}
@@ -646,60 +641,6 @@ public class IncludeCreator {
 		return chain;
 	}
 
-	/**
-	 * Given a header file, decides if this header file should be included directly or
-	 * through another header file. For example, <code>bits/stl_map.h</code> is not supposed
-	 * to be included directly, but should be represented by <code>map</code>.
-	 * @return the header file to include.
-	 */
-	private IIndexFile getRepresentativeFile(IIndexFile headerFile, IIndex index) {
-		try {
-			if (isWorkspaceFile(headerFile.getLocation().getURI())) {
-				return headerFile;
-			}
-			ArrayDeque<IIndexFile> front = new ArrayDeque<>();
-			front.add(headerFile);
-			HashSet<IIndexFile> processed = new HashSet<>();
-			processed.add(headerFile);
-			while (!front.isEmpty()) {
-				IIndexFile file = front.remove();
-				// A header without an extension is a good candidate for inclusion into a C++ source
-				// file.
-				if (fContext.isCXXLanguage() && !hasExtension(getPath(file))) {
-					return file;
-				}
-				IIndexInclude[] includes = index.findIncludedBy(file, 0);
-				for (IIndexInclude include : includes) {
-					IIndexFile includer = include.getIncludedBy();
-					if (!processed.contains(includer)) {
-						URI uri = includer.getLocation().getURI();
-						if (isSource(uri.getPath()) || isWorkspaceFile(uri)) {
-							return file;
-						}
-						front.add(includer);
-						processed.add(includer);
-					}
-				}
-			}
-		} catch (CoreException e) {
-			CUIPlugin.log(e);
-		}
-		return headerFile;
-	}
-
-	private boolean isWorkspaceFile(URI uri) {
-		for (IFile file : ResourceLookup.findFilesForLocationURI(uri)) {
-			if (file.exists()) {
-				return true;
-			}
-		}
-		return false;
-	}
-
-	private boolean hasExtension(String path) {
-		return path.indexOf('.', path.lastIndexOf('/') + 1) >= 0;
-	}
-
 	private IFunctionSummary findContribution(final String name) throws CoreException {
 		ICHelpInvocationContext context = new ICHelpInvocationContext() {
 			@Override
@@ -716,91 +657,6 @@ public class IncludeCreator {
 		return CHelpProviderManager.getDefault().getFunctionInfo(context, name);
 	}
 
-	/**
-	 * Checks if a file is a source file (.c, .cpp, .cc, etc). Header files are not considered
-	 * source files.
-	 *
-	 * @return Returns {@code true} if the the file is a source file.
-	 */
-	private boolean isSource(String filename) {
-		IContentType ct= CCorePlugin.getContentType(fContext.getProject(), filename);
-		if (ct != null) {
-			String id = ct.getId();
-			if (CCorePlugin.CONTENT_TYPE_CSOURCE.equals(id) || CCorePlugin.CONTENT_TYPE_CXXSOURCE.equals(id)) {
-				return true;
-			}
-		}
-		return false;
-	}
-
-	private static String getPath(IIndexFile file) throws CoreException {
-		return file.getLocation().getURI().getPath();
-	}
-
-	/**
-	 * Returns the {@link IncludeInfo} object to be added to the include list
-	 *
-	 * @param path - the full path of the file to include
-	 * @return the {@link IncludeInfo} object
-	 * @throws CoreException 
-	 */
-	private IncludeInfo getIncludeByHeuristic(IIndexFile file, IIndex index) throws CoreException {
-		file = getRepresentativeFile(file, index);
-		IIndexInclude[] includes = index.findIncludedBy(file);
-		if (includes.length > 0) {
-			// Let the existing includes vote. To be eligible to vote, an include
-			// has to be resolvable in the context of the current translation unit.
-			int systemIncludeVotes = 0;
-			String[] ballotBox = new String[includes.length];
-			int k = 0;
-			for (IIndexInclude include : includes) {
-				if (isResolvableInCurrentContext(include)) {
-					ballotBox[k++] = include.getFullName();
-					if (include.isSystemInclude()) {
-						systemIncludeVotes++;
-					}
-				}
-			}
-			if (k != 0) {
-				Arrays.sort(ballotBox, 0, k);
-				String contender = ballotBox[0];
-				int votes = 1;
-				String winner = contender;
-				int winnerVotes = votes;
-				for (int i = 1; i < k; i++) {
-					if (!ballotBox[i].equals(contender)) {
-						contender = ballotBox[i]; 
-						votes = 1;
-					}
-					votes++;
-					if (votes > winnerVotes) {
-						winner = contender;
-						winnerVotes = votes;
-					}
-				}
-				return new IncludeInfo(winner, systemIncludeVotes * 2 >= k);
-			}
-		}
-
-		// The file has never been included before.
-        IPath targetLocation = getAbsolutePath(file.getLocation());
-        return fContext.getIncludeForHeaderFile(targetLocation);
-    }
-
-	/**
-	 * Returns {@code true} if the given include can be resolved in the context of
-	 * the current translation unit.
-	 */
-	private boolean isResolvableInCurrentContext(IIndexInclude include) {
-		try {
-			IncludeInfo includeInfo = new IncludeInfo(include.getFullName(), include.isSystemInclude());
-			return fContext.resolveInclude(includeInfo) != null;
-		} catch (CoreException e) {
-			CUIPlugin.log(e);
-			return false;
-		}
-	}
-
 	/**
 	 * Returns the fully qualified name for a given index binding.
 	 *

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeOrganizer.java

@@ -175,8 +175,8 @@ public class IncludeOrganizer {
 
 	/**
 	 * Organizes the includes for a given translation unit.
+	 *
 	 * @param ast The AST translation unit to process.
-	 * @throws CoreException
 	 */
 	public MultiTextEdit organizeIncludes(IASTTranslationUnit ast) throws CoreException {
 		// Process the given translation unit with the inclusion resolver.
@@ -333,7 +333,6 @@ public class IncludeOrganizer {
 
 	/**
 	 * Creates forward declarations by examining the list of bindings which have to be declared.
-	 * @param pendingBlankLine
 	 */
 	private void createForwardDeclarations(IASTTranslationUnit ast, BindingClassifier classifier,
 			int offset, boolean pendingBlankLine, MultiTextEdit rootEdit)	throws CoreException {
@@ -888,7 +887,7 @@ public class IncludeOrganizer {
 				Map<IIndexFile, IPath> reachableDeclaringHeaders = new HashMap<>();
 				for (IIndexName indexName : indexNames) {
 					IIndexFile indexFile = indexName.getFile();
-					if (!canBeIncluded(indexFile)) {
+					if (!fContext.canBeIncluded(indexFile)) {
 						// The target is a source file which isn't included by any other files.
 						// Don't include it.
 						continue;
@@ -918,17 +917,12 @@ public class IncludeOrganizer {
 		int pos = 0;
 		for (IIndexName name : names) {
 			IIndexFile file = name.getFile();
-			if (file != null && !blacklist.contains(file) && canBeIncluded(file))
+			if (file != null && !blacklist.contains(file) && fContext.canBeIncluded(file))
 				includable = ArrayUtil.appendAt(includable, pos++, name);
 		}
 		return ArrayUtil.trim(includable, pos);
 	}
 
-	private boolean canBeIncluded(IIndexFile indexFile) throws CoreException {
-		return !IncludeUtil.isSource(indexFile, fContext.getProject()) ||
-				fContext.getIndex().findIncludedBy(indexFile, 0).length != 0;
-	}
-
 	private String createIncludeDirective(IncludePrototype include, String lineComment) {
 		StringBuilder buf = new StringBuilder();
 		// Unresolved includes are preserved out of caution. Partner include is always preserved.