Improve formatter problem reporting

core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/formatter/AbortFormatting.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2006 IBM Corporation and others.
+ * Copyright (c) 2000, 2007 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
@@ -25,8 +25,7 @@ public class AbortFormatting extends RuntimeException {
 	public AbortFormatting(String message) {
 		super(message);
 	}
-	public AbortFormatting(Throwable nestedException) {
+	public AbortFormatting(Throwable cause) {
-		super(nestedException.getMessage());
+		super(cause);
-		this.nestedException = nestedException;
 	}
 }

core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/formatter/CodeFormatterVisitor.java

@@ -115,7 +115,10 @@ import org.eclipse.cdt.internal.formatter.align.Alignment;
 import org.eclipse.cdt.internal.formatter.align.AlignmentException;
 import org.eclipse.cdt.internal.formatter.scanner.Scanner;
 import org.eclipse.cdt.internal.formatter.scanner.Token;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.MultiStatus;
 import org.eclipse.core.runtime.Platform;
+import org.eclipse.core.runtime.Status;
 import org.eclipse.jface.text.Position;
 import org.eclipse.text.edits.TextEdit;
 
@@ -129,13 +132,25 @@ import org.eclipse.text.edits.TextEdit;
  */
 public class CodeFormatterVisitor extends CPPASTVisitor {
 
-	private static boolean DEBUG = "true".equalsIgnoreCase(Platform.getDebugOption("org.eclipse.cdt.core/debug/formatter"));
+	private static boolean DEBUG = "true".equalsIgnoreCase(Platform.getDebugOption("org.eclipse.cdt.core/debug/formatter")); //$NON-NLS-1$ //$NON-NLS-2$
 
 	private static class ASTProblemException extends RuntimeException {
 		private static final long serialVersionUID= 1L;
+		private IASTProblem fProblem;
 		ASTProblemException(IASTProblem problem) {
-			super(problem.getMessage());
+			super();
-			if (DEBUG) System.err.println("PROBLEM: "+getMessage());
+			fProblem= problem;
+		}
+		/*
+		 * @see java.lang.Throwable#getMessage()
+		 */
+		public String getMessage() {
+			String message= fProblem.getMessage();
+			if (fProblem.getFileLocation() != null) {
+				int line= fProblem.getFileLocation().getStartingLineNumber();
+				message += " (line " + line + ')'; //$NON-NLS-1$
+			}
+			return message;
 		}
 	}
 
@@ -179,6 +194,8 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 
 	private boolean fInsideFor;
 
+	private MultiStatus fStatus;
+
 	public CodeFormatterVisitor(DefaultCodeFormatterOptions preferences, Map settings, int offset, int length) {
 		localScanner = new Scanner() {
 			public Token nextToken() {
@@ -210,32 +227,26 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 		scribe.setSkipPositions(collectInactiveCodePositions(unit));
 
 		fTranslationUnitFile= unit.getFilePath();
+		fStatus= new MultiStatus(CCorePlugin.PLUGIN_ID, 0, "Formatting problem(s)", null); //$NON-NLS-1$
 		try {
 			unit.accept(this);
-		} catch (AbortFormatting e){
-			return failedToFormat(e);
 		} catch (RuntimeException e) {
-			return failedToFormat(new AbortFormatting(e));
+			reportFormattingProblem(e);
+			if (DEBUG) return failedToFormat(e);
 		}
 		if (DEBUG){
 			System.out.println("Formatting time: " + (System.currentTimeMillis() - startTime));  //$NON-NLS-1$
 		}
+		if (!fStatus.isOK()) {
+			CCorePlugin.log(fStatus);
+		}
 		return scribe.getRootEdit();
 	}
 
-	private final TextEdit failedToFormat(AbortFormatting e) {
+	private final TextEdit failedToFormat(RuntimeException e) {
-		String errorMessage= e.getMessage();
-		if (errorMessage == null) {
-			if (e.nestedException != null) {
-				errorMessage= e.nestedException.getClass().getName();
-			} else {
-				errorMessage= "Unknown error";
-			}
-		}
-		CCorePlugin.log(CCorePlugin.createStatus("Could not format: " + errorMessage, e.nestedException));
 		if (DEBUG) {
-			System.out.println("COULD NOT FORMAT: " + e.getMessage());
+			System.out.println("COULD NOT FORMAT: " + e.getMessage()); //$NON-NLS-1$
-			System.out.println(scribe.scanner); //$NON-NLS-1$
+			System.out.println(scribe.scanner);
 			System.out.println(scribe);
 			System.out.flush();
 			System.err.flush();
@@ -245,6 +256,18 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 		return null;
 	}
 
+	private void reportFormattingProblem(RuntimeException e) {
+		String errorMessage= e.getMessage();
+		if (errorMessage == null) {
+			errorMessage= "Unknown error"; //$NON-NLS-1$
+		}
+		fStatus.add(createStatus(errorMessage, e));
+	}
+
+	private static IStatus createStatus(String msg, Throwable e) {
+		return new Status(IStatus.WARNING, CCorePlugin.PLUGIN_ID, msg, e);
+	}
+
 	/*
 	 * @see org.eclipse.cdt.core.dom.ast.ASTVisitor#visit(org.eclipse.cdt.core.dom.ast.IASTTranslationUnit)
 	 */
@@ -252,7 +275,7 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 		// fake new line
 		scribe.lastNumberOfNewLines = 1;
 		scribe.startNewLine();
-		int indentLevel= scribe.indentationLevel;
+		final int indentLevel= scribe.indentationLevel;
 		IASTDeclaration[] decls= tu.getDeclarations();
 		for (int i = 0; i < decls.length; i++) {
 			IASTDeclaration declaration = decls[i];
@@ -262,7 +285,9 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 			try {
 				declaration.accept(this);
 				scribe.startNewLine();
-			} catch (ASTProblemException e) {
+			} catch (RuntimeException e) {
+				// report, but continue
+				reportFormattingProblem(e);
 				if (i < decls.length - 1) {
 					exitAlignments();
 					skipToNode(decls[i+1]);
@@ -876,8 +901,7 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 			final ListAlignment align= new ListAlignment(Alignment.M_COMPACT_SPLIT);
 			formatList(declarators, align, false, false);
 		}
-		if (peekNextToken() == Token.tIDENTIFIER) {
+		if (peekNextToken() != Token.tSEMI) {
-			// there may be a macro
 			scribe.skipToToken(Token.tSEMI);
 		}
 		scribe.printNextToken(Token.tSEMI, preferences.insert_space_before_semicolon);
@@ -1138,6 +1162,8 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 					ok = true;
 				} catch (AlignmentException e) {
 					scribe.redoAlignment(e);
+				} catch (ASTProblemException e) {
+					
 				}
 			} while (!ok);
 			scribe.exitAlignment(listAlignment, true);
@@ -1231,6 +1257,11 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 		scribe.printNextToken(peekNextToken());
 		scribe.printNextToken(Token.tLPAREN);
 		node.getTypeId().accept(this);
+		if (peekNextToken() == Token.tCOMMA) {
+			scribe.printNextToken(Token.tCOMMA, preferences.insert_space_before_comma_in_method_invocation_arguments);
+			scribe.printNextToken(peekNextToken(), preferences.insert_space_after_comma_in_method_invocation_arguments);
+			scribe.skipToToken(Token.tRPAREN);
+		}
 		scribe.printNextToken(Token.tRPAREN);
     	return PROCESS_SKIP;
 	}
@@ -1301,34 +1332,32 @@ public class CodeFormatterVisitor extends CPPASTVisitor {
 	}
 
 	private int visit(IASTLiteralExpression node) {
-		if (node.getKind() == IASTLiteralExpression.lk_string_literal) {
+		if (node.getNodeLocations().length > 1) {
+			// cannot handle embedded macros
+			skipNode(node);
+		} else if (node.getKind() == IASTLiteralExpression.lk_string_literal) {
 			// handle concatentation of string literals
-			if (node.getNodeLocations().length > 1) {
+			int token;
-				// cannot handle embedded macros
+			boolean needSpace= false;
-				skipNode(node);
+			final int line= scribe.line;
-			} else {
+			boolean indented= false;
-				int token;
+			try {
-				boolean needSpace= false;
+				while (true) {
-				final int line= scribe.line;
+					scribe.printNextToken(Token.tSTRING, needSpace);
-				boolean indented= false;
+					token= peekNextToken();
-				try {
+					if (token != Token.tSTRING) {
-					while (true) {
+						break;
-						scribe.printCommentPreservingNewLines();
-						scribe.printNextToken(Token.tSTRING, needSpace);
-						token= peekNextToken();
-						if (token != Token.tSTRING) {
-							break;
-						}
-						needSpace= true;
-						if (!indented && line != scribe.line) {
-							indented= true;
-							scribe.indent();
-						}
 					}
-				} finally {
+					scribe.printCommentPreservingNewLines();
-					if (indented) {
+					if (!indented && line != scribe.line) {
-						scribe.unIndent();
+						indented= true;
+						scribe.indent();
 					}
+					needSpace= true;
+				}
+			} finally {
+				if (indented) {
+					scribe.unIndent();
 				}
 			}
 		} else {

core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/formatter/Scribe.java

@@ -776,6 +776,9 @@ public class Scribe {
 
 			switch (currentCharacter) {
 			case '\r':
+				if (isNewLine) {
+					line++;
+				}
 				start= previousStart;
 				isNewLine= true;
 				if (scanner.getNextChar('\n')) {
@@ -784,6 +787,9 @@ public class Scribe {
 				}
 				break;
 			case '\n':
+				if (isNewLine) {
+					line++;
+				}
 				start= previousStart;
 				isNewLine= true;
 				break;
@@ -865,7 +871,7 @@ public class Scribe {
 			previousStart= nextCharacterStart;
 			scanner.setCurrentPosition(nextCharacterStart);
 		}
-		lastNumberOfNewLines= 0;
+		lastNumberOfNewLines= isNewLine ? 1 : 0;
 		needSpace= false;
 		scanner.resetTo(currentTokenEndPosition, scannerEndPosition - 1);
 	}
@@ -1232,7 +1238,7 @@ public class Scribe {
 		currentToken= scanner.nextToken();
 		if (currentToken == null || expectedTokenType != currentToken.type) {
 			throw new AbortFormatting(
-					"["	+ line + "/" + column + "] unexpected token type, expecting:" + expectedTokenType + ", actual:" + currentToken);//$NON-NLS-1$//$NON-NLS-2$
+					"["	+ (line+1) + "/" + column + "] unexpected token type, expecting:" + expectedTokenType + ", actual:" + currentToken);//$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
 		}
 		print(currentToken.getLength(), considerSpaceIfAny);
 	}
@@ -1253,7 +1259,7 @@ public class Scribe {
 				expectations.append(expectedTokenTypes[i]);
 			}
 			throw new AbortFormatting(
-					"unexpected token type, expecting:[" + expectations.toString() + "], actual:" + currentToken);//$NON-NLS-1$//$NON-NLS-2$
+					"["	+ (line+1) + "/" + column + "] unexpected token type, expecting:[" + expectations.toString() + "], actual:" + currentToken);//$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
 		}
 		print(currentToken.getLength(), considerSpaceIfAny);
 	}
@@ -1521,6 +1527,24 @@ public class Scribe {
 				hasComment= false;
 				break;
 			default:
+				if (currentToken.getType() == Token.tIDENTIFIER) {
+					if (currentToken.getText().startsWith("__")) { //$NON-NLS-1$
+						// assume this is a declspec modifier
+						print(currentToken.getLength(), !isFirstModifier);
+						isFirstModifier= false;
+						currentTokenStartPosition= scanner.getCurrentPosition();
+						if ((currentToken= scanner.nextToken()) != null) {
+							if (currentToken.getType() == Token.tLPAREN) {
+								if (skipToToken(Token.tRPAREN)) {
+									currentToken= scanner.nextToken();
+									currentToken= scanner.nextToken();
+									currentTokenStartPosition= scanner.getCurrentPosition();
+								}
+							}
+						}
+						break;
+					}
+				}
 				// step back one token
 				scanner.resetTo(currentTokenStartPosition, scannerEndPosition - 1);
 				return;

core/org.eclipse.cdt.ui.tests/resources/formatter/bugs/After.cpp

@@ -27,3 +27,8 @@ int main(int argc, char **argv) {
 char* s1= "this " "is " "one " "string.";
 char* s2= "this " "is "
 	"one " "string.";
+// macro definition with line comment
+#define ID(x) x  // identity
+int main() {
+	return ID(0);
+}

core/org.eclipse.cdt.ui.tests/resources/formatter/bugs/Before.cpp

@@ -9,9 +9,11 @@ class AClass : public ABaseClass {AClass(int x) throw(int); void test1() const t
 AClass::AClass(int x)throw(int):ABaseClass(x){for (int i=0;i < 12;i++) {}}
 // keep space between decl spec and declarator
 int
-main(int argc, char **argv) {
+main(int argc, char **argv) {}
-}
 // handling of string concat
 char* s1= "this "   "is "  "one ""string.";
 char* s2= "this " "is " 
 "one " "string.";
+// macro definition with line comment
+#define ID(x) x  // identity
+int main() {return ID(0);}

core/org.eclipse.cdt.ui.tests/resources/formatter/sample/After.cpp

@@ -8,9 +8,9 @@ struct SimpleStruct {
 	float floatNum;
 };
 
-void SimpleStruct_construct( struct SimpleStruct * const this );
+void SimpleStruct_construct(struct SimpleStruct * const s);
 
-int SimpleStruct_doSomething( const struct SimpleStruct * const this );
+int SimpleStruct_doSomething(const struct SimpleStruct * const s);
 
 #endif /* SIMPLE_H */
 
@@ -34,17 +34,14 @@ const SimpleStruct array[] = { { SIZEOF( simpleStruct, num ),
 
 //          single line outside scope
 
-void SimpleStruct_construct(
+void SimpleStruct_construct(struct SimpleStruct * const s) {
-		struct SimpleStruct * const this )
-{
 	// single line
-	this->num = 1;
+	s->num = 1;
-	this->name = "boo";
+	s->name = "boo";
-	this->floatNum = 1.5;
+	s->floatNum = 1.5;
 }
 
-int ConnectParams_doSomething( const struct SimpleStruct * const this )
+int ConnectParams_doSomething(const struct SimpleStruct * const s) {
-{
 	/*
 	 * multiline
 	 */

core/org.eclipse.cdt.ui.tests/resources/formatter/sample/Before.cpp

@@ -10,9 +10,9 @@ struct SimpleStruct
 };
 
 
-void SimpleStruct_construct( struct SimpleStruct * const this );
+void SimpleStruct_construct( struct SimpleStruct * const s );
 
-int SimpleStruct_doSomething( const struct SimpleStruct * const this );
+int SimpleStruct_doSomething( const struct SimpleStruct * const s );
 
 #endif /* SIMPLE_H */
 
@@ -53,15 +53,15 @@ const SimpleStruct array[] =
 //          single line outside scope
 
 void SimpleStruct_construct( 
-struct SimpleStruct * const this )
+struct SimpleStruct * const s )
   {
 // single line
-this->num = 1;
+s->num = 1;
-this->name = "boo";
+s->name = "boo";
-this->floatNum = 1.5;
+s->floatNum = 1.5;
   }
 
-int ConnectParams_doSomething( const struct SimpleStruct * const this )
+int ConnectParams_doSomething( const struct SimpleStruct * const s )
     {
 /*
  * multiline

core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/FormatActionTest.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2006 IBM Corporation and others.
+ * Copyright (c) 2005, 2007 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
@@ -132,6 +132,7 @@ public class FormatActionTest extends TestCase {
 	}
 
 	public void testBugs() throws Exception {
-		
+		selectAll();
+		assertFormatResult();
 	}
 }