Bug 333813 - fixed fp, default settings and description

codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties

@@ -58,5 +58,5 @@ problem.name.SuspiciousSemicolon = Suspicious semicolon
 problem.messagePattern.SuspiciousSemicolon = Suspicious semicolon
 problem.description.SuspiciousSemicolon = A semicolon is used as a null statement in a condition. For example, 'if(expression);'
 checker.name.CaseBreak = No break at end of case
-problem.description.CaseBreak = Looks for "case" statements which end without a "break" statement statement statement
+problem.description.CaseBreak = Looks for "case" statements which end without a "break" statement
 problem.messagePattern.CaseBreak = No break at the end of this case

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CaseBreakChecker.java

@@ -130,62 +130,72 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements
 	 * "switch"s
 	 */
 	class SwitchVisitor extends SwitchFindingVisitor {
-		private IASTStatement _last_case_stmnt;
+		private IASTStatement _prev_case_stmnt;
 		private boolean _first_case_statement;
-		private int _last_break_stmnt_offset;
-		private int _last_normal_stmnt_offset;
-		private int _last_case_stmnt_offset;
+		private int _prev_break_stmnt_offset;
+		private int _prev_normal_stmnt_offset;
+		private int _prev_case_stmnt_offset;
 
 		SwitchVisitor(IASTStatement switch_statement) {
 			shouldVisitStatements = true;
 			_first_case_statement = true;
 			_switchStatement = switch_statement;
-			_last_break_stmnt_offset = 0;
-			_last_normal_stmnt_offset = 0;
-			_last_case_stmnt_offset = 0;
-			_last_case_stmnt = null;
+			_prev_break_stmnt_offset = 0;
+			_prev_normal_stmnt_offset = 0;
+			_prev_case_stmnt_offset = 0;
+			_prev_case_stmnt = null;
 		}
 
 		/**
 		 * @return Is this an "empty" case (i.e. with no statements in it)
 		 */
 		private boolean isEmptyCase() {
-			return _last_case_stmnt_offset > _last_normal_stmnt_offset;
+			return _prev_case_stmnt_offset > _prev_normal_stmnt_offset;
 		}
 
 		/**
 		 * @return Was a "break" statement the last statement in this case
 		 */
-		private boolean breakFoundLast() {
-			return _last_normal_stmnt_offset < _last_break_stmnt_offset
-					&& _last_case_stmnt_offset < _last_break_stmnt_offset;
+		private boolean breakFoundPrevious() {
+			return _prev_normal_stmnt_offset < _prev_break_stmnt_offset
+					&& _prev_case_stmnt_offset < _prev_break_stmnt_offset;
 		}
 
 		/**
 		 * Check the last case we've visited
 		 * 
 		 * @param comment The comment ending this case (may be NULL)
+		 * @param lastOne true if it am actual Last statement
 		 */
-		private void checkLastCase(IASTComment comment) {
+		private void checkPreviousCase(IASTComment comment, boolean lastOne) {
 			if (comment != null) {
-				String str = new String(comment.getComment());
-				if (comment.isBlockComment())
-					str = str.substring(2, str.length() - 2);
-				else
-					str = str.substring(2);
-				str = str.trim();
+				String str = getTrimmedComment(comment);
 				if (str.equalsIgnoreCase(_noBreakComment))
 					return;
 			}
-			if (_last_case_stmnt == null)
+			if (_prev_case_stmnt == null)
 				return; // This is an empty switch
-			if (breakFoundLast())
+			if (breakFoundPrevious())
 				return; // There was a "break" before the current statement 
-			if (!isEmptyCase() || _checkEmptyCase) {
-				reportProblem(ER_ID, _last_case_stmnt, (Object) null);
+			if (lastOne == true || !isEmptyCase() || _checkEmptyCase) {
+				reportProblem(ER_ID, _prev_case_stmnt, (Object) null);
 			}
 		}
 
+		/**
+		 * @param comment
+		 * @return
+		 */
+		public String getTrimmedComment(IASTComment comment) {
+			String str = new String(comment.getComment());
+			if (comment.isBlockComment())
+				str = str.substring(2, str.length() - 2);
+			else
+				str = str.substring(2);
+			str = str.trim();
+			return str;
+		}
+
 		@Override
 		public int visit(IASTStatement statement) {
 			if (statement instanceof IASTCaseStatement
@@ -213,17 +223,17 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements
 					 * 'comment' is the last comment found in this case (after
 					 * the last statement in this "case"
 					 */
-					checkLastCase(comment);
+					checkPreviousCase(comment, false);
 				}
 				/* Update variables with the new opened "case" */
-				_last_case_stmnt_offset = statement.getFileLocation()
+				_prev_case_stmnt_offset = statement.getFileLocation()
 						.getNodeOffset();
-				_last_case_stmnt = statement;
+				_prev_case_stmnt = statement;
 			} else if (isBreakOrExitStatement(statement)) { // A "break" statement
-				_last_break_stmnt_offset = statement.getFileLocation()
+				_prev_break_stmnt_offset = statement.getFileLocation()
 						.getNodeOffset();
 			} else { // a non-switch related statement
-				_last_normal_stmnt_offset = statement.getFileLocation()
+				_prev_normal_stmnt_offset = statement.getFileLocation()
 						.getNodeOffset();
 			}
 			/* advance comments we already passed */
@@ -252,7 +262,7 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements
 				 * 'comment' is the last comment found in this case (after the
 				 * last statement in this "case"
 				 */
-				checkLastCase(comment);
+				checkPreviousCase(comment, true);
 			}
 			return super.leave(statement);
 		}
@@ -284,12 +294,13 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements
 				PARAM_NO_BREAK_COMMENT,
 				CheckersMessages.CaseBreakChecker_DefaultNoBreakCommentDescription,
 				DEFAULT_NO_BREAK_COMMENT);
-		addPreference(problem, PARAM_EMPTY_CASE,
-				CheckersMessages.CaseBreakChecker_EmptyCaseDescription,
-				Boolean.TRUE);
 		addPreference(problem, PARAM_LAST_CASE,
 				CheckersMessages.CaseBreakChecker_LastCaseDescription,
 				Boolean.TRUE);
+		addPreference(problem, PARAM_EMPTY_CASE,
+				CheckersMessages.CaseBreakChecker_EmptyCaseDescription,
+				Boolean.FALSE);
+
 	}
 
 	public void processAst(IASTTranslationUnit ast) {

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/messages.properties

@@ -9,7 +9,7 @@
 #     Alena Laskavaia - initial API and implementation
 ###############################################################################
 CaseBreakChecker_DefaultNoBreakCommentDescription=Comment text to suppress the problem (regular expression): 
-CaseBreakChecker_EmptyCaseDescription=Check also empty case statement
+CaseBreakChecker_EmptyCaseDescription=Check also empty case statement (except if last)
 CaseBreakChecker_LastCaseDescription=Check also the last case statement
 CatchByReference_ReportForUnknownType=Report a problem if type cannot be resolved
 NamingConventionFunctionChecker_LabelNamePattern=Name Pattern

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/CaseBreakCheckerTest.java

@@ -18,6 +18,19 @@ import org.eclipse.cdt.codan.internal.checkers.CaseBreakChecker;
  * Test for {@link#CaseBreakChecker} class
  */
 public class CaseBreakCheckerTest extends CheckerTestCase {
+	/*
+	 * (non-Javadoc)
+	 * 
+	 * @see org.eclipse.cdt.codan.core.test.CodanTestCase#setUp()
+	 */
+	@Override
+	public void setUp() throws Exception {
+		super.setUp();
+		// set default prefs
+		setEmpty(false);
+		setLast(true);
+	}
+
 	// void foo(void) {
 	//  int a;
 	//  switch( a ) {
@@ -62,6 +75,7 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 	//  }
 	// }
 	public void testEmptyCaseBad() {
+		setEmpty(true);
 		loadCodeAndRun(getAboveComment());
 		checkErrorLines(4);
 	}
@@ -293,6 +307,7 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 	//  }
 	// }
 	public void testGeneral1() {
+		setEmpty(true);
 		loadCodeAndRun(getAboveComment());
 		checkErrorLines(4, 6, 7, 11, 14, 16, 19, 20, 24, 27, 32, 37, 41, 46,
 				49, 51);
@@ -394,7 +409,6 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		setEmpty(false);
 		loadCodeAndRun(getAboveComment());
 		checkNoErrors();
-		setEmpty(true);
 	}
 
 	// void foo(void) {
@@ -411,8 +425,7 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		setLast(true);
 		setEmpty(false);
 		loadCodeAndRun(code);
-		checkNoErrors();
-		setEmpty(true);
+		checkErrorLine(4);
 	}
 
 	private void setLast(boolean val) {
@@ -439,4 +452,23 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		checkNoErrors(); // FALSE NEGATIVE
 		//checkErrorLine(3);
 	}
+
+	// void foo(void) {
+	//  int a;
+	//  switch( a ) {
+	//  case 2:
+	//     break;
+	//  case 1:
+	//  }
+	// }
+	public void testEmptyLastCaseError() {
+		String code = getAboveComment();
+		setLast(true);
+		setEmpty(false);
+		loadCodeAndRun(code);
+		checkErrorLine(6);
+		setLast(false);
+		loadCodeAndRun(code);
+		checkNoErrors();
+	}
 }

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CheckerTestCase.java

@@ -27,7 +27,7 @@ import org.eclipse.core.runtime.NullProgressMonitor;
 
 /**
  * @author Alena
- *
+ * 
  */
 public class CheckerTestCase extends CodanTestCase {
 	protected IMarker[] markers;
@@ -36,8 +36,8 @@ public class CheckerTestCase extends CodanTestCase {
 		return checkErrorLine(currentFile, i);
 	}
 
-	public void checkErrorLines( Object... args ) {
-		for( Object i : args ) {
+	public void checkErrorLines(Object... args) {
+		for (Object i : args) {
 			checkErrorLine((Integer) i);
 		}
 		assertEquals(args.length, markers.length);
@@ -65,18 +65,7 @@ public class CheckerTestCase extends CodanTestCase {
 		IMarker m = null;
 		for (int j = 0; j < markers.length; j++) {
 			m = markers[j];
-			Object pos;
-			try {
-				line = (Integer) m.getAttribute(IMarker.LINE_NUMBER);
-				if (line == null || line.equals(-1)) {
-					pos = m.getAttribute(IMarker.CHAR_START);
-					line = new Integer(pos2line(((Integer) pos).intValue()));
-				}
-			} catch (CoreException e) {
-				fail(e.getMessage());
-			} catch (IOException e) {
-				fail(e.getMessage());
-			}
+			line = getLine(m);
 			mfile = m.getResource().getName();
 			if (line.equals(expectedLine)
 					&& (problemId == null || problemId
@@ -96,9 +85,36 @@ public class CheckerTestCase extends CodanTestCase {
 		return m;
 	}
 
+	/**
+	 * @param line
+	 * @param m
+	 * @return
+	 */
+	public Integer getLine(IMarker m) {
+		Integer line = null;
+		try {
+			line = (Integer) m.getAttribute(IMarker.LINE_NUMBER);
+			if (line == null || line.equals(-1)) {
+				Object pos = m.getAttribute(IMarker.CHAR_START);
+				line = new Integer(pos2line(((Integer) pos).intValue()));
+			}
+		} catch (CoreException e) {
+			fail(e.getMessage());
+		} catch (IOException e) {
+			fail(e.getMessage());
+		}
+		return line;
+	}
+
 	public void checkNoErrors() {
-		assertTrue("Found errors but should not", markers == null
-				|| markers.length == 0);
+		if (markers == null || markers.length == 0) {
+			// all good
+		} else {
+			IMarker m = markers[0];
+			fail("Found " + markers.length + " errors but should not. First "
+					+ CodanProblemMarker.getProblemId(m) + " at line "
+					+ getLine(m));
+		}
 	}
 
 	/**