Bug 343676: rewrote case checker to support complex structures

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

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2010 Gil Barash 
+ * Copyright (c) 2010,2011 Gil Barash 
  * 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:
  *    Gil Barash  - Initial implementation
+ *    Elena laskavaia - Rewrote checker to reduce false positives in complex cases
  *******************************************************************************/
 package org.eclipse.cdt.codan.internal.checkers;
 
@@ -21,8 +22,9 @@ import org.eclipse.cdt.core.dom.ast.IASTComment;
 import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement;
 import org.eclipse.cdt.core.dom.ast.IASTContinueStatement;
 import org.eclipse.cdt.core.dom.ast.IASTDefaultStatement;
+import org.eclipse.cdt.core.dom.ast.IASTExpressionStatement;
 import org.eclipse.cdt.core.dom.ast.IASTGotoStatement;
-import org.eclipse.cdt.core.dom.ast.IASTNode;
+import org.eclipse.cdt.core.dom.ast.IASTIfStatement;
 import org.eclipse.cdt.core.dom.ast.IASTReturnStatement;
 import org.eclipse.cdt.core.dom.ast.IASTStatement;
 import org.eclipse.cdt.core.dom.ast.IASTSwitchStatement;
@@ -43,25 +45,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 	 * them
 	 */
 	class SwitchFindingVisitor extends ASTVisitor {
-		protected IASTStatement _switchStatement; // The "switch" we're visiting (used by inheriting classes to avoid re-visiting the same "switch" again)
-
 		SwitchFindingVisitor() {
 			shouldVisitStatements = true;
-			_switchStatement = null;
-		}
-
-		/**
-		 * @param statement
-		 * @return true iff the statement is directly under the "switch" and not
-		 *         in the scope of some loop statement, such as "while".
-		 */
-		private boolean doesStatementAffectThisSwitch(IASTStatement statement) {
-			IASTNode parent = statement.getParent();
-			if (parent == _switchStatement)
-				return true;
-			if (parent instanceof IASTCompoundStatement)
-				return doesStatementAffectThisSwitch((IASTCompoundStatement) parent);
-			return false;
 		}
 
 		/**
@@ -78,165 +63,121 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 		 */
 		protected boolean isBreakOrExitStatement(IASTStatement statement) {
 			CxxAstUtils utils = CxxAstUtils.getInstance();
-			return (statement instanceof IASTBreakStatement && doesStatementAffectThisSwitch(statement))
+			return (statement instanceof IASTBreakStatement) || statement instanceof IASTReturnStatement
-					|| statement instanceof IASTReturnStatement || statement instanceof IASTContinueStatement
+					|| statement instanceof IASTContinueStatement || statement instanceof IASTGotoStatement
-					|| statement instanceof IASTGotoStatement || utils.isThrowStatement(statement) || utils.isExitStatement(statement);
+					|| utils.isThrowStatement(statement) || utils.isExitStatement(statement);
 		}
 
 		@Override
 		public int visit(IASTStatement statement) {
 			if (statement instanceof IASTSwitchStatement) {
-				// Are we already visiting this statement?
+				IASTSwitchStatement switchStmt = (IASTSwitchStatement) statement;
-				if (_switchStatement == null || !statement.equals(_switchStatement)) {
+				IASTStatement body = switchStmt.getBody();
-					SwitchVisitor switch_visitor = new SwitchVisitor(statement);
+				if (body instanceof IASTCompoundStatement) {
-					statement.accept(switch_visitor);
+					// if not it is not really a switch
-					return PROCESS_SKIP;
+					IASTStatement[] statements = ((IASTCompoundStatement) body).getStatements();
+					IASTStatement prevCase = null;
+					for (int i = 0; i < statements.length; i++) {
+						IASTStatement curr = statements[i];
+						if (curr instanceof IASTSwitchStatement) {
+							visit(curr);
+						}
+						IASTStatement next = null;
+						if (i < statements.length - 1)
+							next = statements[i + 1];
+						if (isCaseStatement(curr)) {
+							prevCase = curr;
+						}
+						// next is case or end of switch - means this one is the last
+						if (prevCase != null && (isCaseStatement(next) || next == null)) {
+							// check that current statement end with break or any other exit statement
+							if (_checkEmptyCase == false && isCaseStatement(curr) && next != null) {
+								continue; // empty case & we don't care
+							}
+							if (_checkLastCase == false && next == null) {
+								continue; // last case and we don't care
+							}
+							if (isFallThroughStamement(curr)) {
+								IASTComment comment = null;
+								if (next != null) {
+									comment = getLeadingComment(next);
+								} else {
+									comment = getFreestandingComment(statement);
+									if (comment==null)
+										comment = getFreestandingComment(body);
+								}
+								if (comment != null) {
+									String str = getTrimmedComment(comment);
+									if (str.equalsIgnoreCase(_noBreakComment))
+										continue;
+								}
+								reportProblem(ER_ID, prevCase, (Object) null);
+							}
+						}
+					}
 				}
+				return PROCESS_SKIP;
 			}
 			return PROCESS_CONTINUE;
 		}
+
+		/**
+		 * @param statement
+		 * @return
+		 */
+		public boolean isCaseStatement(IASTStatement statement) {
+			return statement instanceof IASTCaseStatement || statement instanceof IASTDefaultStatement;
+		}
+
+		/**
+		 * @param nextstatement
+		 * @return
+		 */
+		public boolean isFallThroughStamement(IASTStatement body) {
+			if (body==null) return true;
+			if (body instanceof IASTCompoundStatement) {
+				IASTStatement[] statements = ((IASTCompoundStatement) body).getStatements();
+				return isFallThroughStamement(statements[statements.length - 1]);
+			} else if (isBreakOrExitStatement(body)) {
+				return false;
+			} else if (body instanceof IASTExpressionStatement) {
+				return true;
+			} else if (body instanceof IASTIfStatement) {
+				IASTIfStatement ifs = (IASTIfStatement) body;
+				return isFallThroughStamement(ifs.getThenClause()) || isFallThroughStamement(ifs.getElseClause()) ;
+			}
+			return true; // TODO
+		}
 	}
 
 	/**
-	 * This visitor visits a switch statement and checks the end of each
+	 * @param comment
-	 * "case" statement (to see that it ends with a "break").
+	 * @return
-	 * Because it extends SwitchFindingVisitor is would also check nested
-	 * "switch"s
 	 */
-	class SwitchVisitor extends SwitchFindingVisitor {
+	public String getTrimmedComment(IASTComment comment) {
-		private IASTStatement _prev_case_stmnt;
+		String str = new String(comment.getComment());
-		private boolean _first_case_statement;
+		if (comment.isBlockComment())
-		private int _prev_break_stmnt_offset;
+			str = str.substring(2, str.length() - 2);
-		private int _prev_normal_stmnt_offset;
+		else
-		private int _prev_case_stmnt_offset;
+			str = str.substring(2);
+		str = str.trim();
+		return str;
+	}
 
-		SwitchVisitor(IASTStatement switch_statement) {
+	/**
-			shouldVisitStatements = true;
+	 * @param statement
-			_first_case_statement = true;
+	 * @return
-			_switchStatement = switch_statement;
+	 */
-			_prev_break_stmnt_offset = 0;
+	public IASTComment getLeadingComment(IASTStatement statement) {
-			_prev_normal_stmnt_offset = 0;
+		return getCommentMap().getLastLeadingCommentForNode(statement);
-			_prev_case_stmnt_offset = 0;
+	}
-			_prev_case_stmnt = null;
-			getCommentMap();
-		}
 
-		/**
+	/**
-		 * @return Is this an "empty" case (i.e. with no statements in it)
+	 * @param statement
-		 */
+	 * @return
-		private boolean isEmptyCase() {
+	 */
-			return _prev_case_stmnt_offset > _prev_normal_stmnt_offset;
+	public IASTComment getFreestandingComment(IASTStatement statement) {
-		}
+		return getCommentMap().getLastFreestandingCommentForNode(statement);
-
-		/**
-		 * @return Was a "break" statement the last statement in this case
-		 */
-		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 checkPreviousCase(IASTComment comment, boolean lastOne) {
-			if (comment != null) {
-				String str = getTrimmedComment(comment);
-				if (str.equalsIgnoreCase(_noBreakComment))
-					return;
-			}
-			if (_prev_case_stmnt == null)
-				return; // This is an empty switch
-			if (breakFoundPrevious())
-				return; // There was a "break" before the current statement 
-			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 || statement instanceof IASTDefaultStatement) {
-				if (_first_case_statement) {
-					/*
-					 * This is the first "case", i.e. the beginning of the
-					 * "switch"
-					 */
-					_first_case_statement = false;
-				} else {
-					/*
-					 * This is not the 1st "case", meaning that a previous case
-					 * has just ended,
-					 * Let's check that case and see how it ended...
-					 */
-					IASTComment comment = getLeadingComment(statement);
-					/*
-					 * 'comment' is the last comment found in this case (after
-					 * the last statement in this "case"
-					 */
-					checkPreviousCase(comment, false);
-				}
-				/* Update variables with the new opened "case" */
-				_prev_case_stmnt_offset = statement.getFileLocation().getNodeOffset();
-				_prev_case_stmnt = statement;
-			} else if (isBreakOrExitStatement(statement)) { // A relevant "break" statement
-				_prev_break_stmnt_offset = statement.getFileLocation().getNodeOffset();
-			} else { // a non-switch related statement
-				_prev_normal_stmnt_offset = statement.getFileLocation().getNodeOffset();
-			}
-			return super.visit(statement); // This would handle nested "switch"s
-		}
-
-		/**
-		 * @param statement
-		 * @return
-		 */
-		public IASTComment getLeadingComment(IASTStatement statement) {
-			return getCommentMap().getLastLeadingCommentForNode(statement);
-		}
-
-
-		/**
-		 * @param statement
-		 * @return
-		 */
-		public IASTComment getFreestandingComment(IASTStatement statement) {
-			return getCommentMap().getLastFreestandingCommentForNode(statement);
-		}
-
-		@Override
-		public int leave(IASTStatement statement) {
-			/*
-			 * Are we leaving the "switch" altogether? (we need to see how the
-			 * last "case" ended)
-			 */
-			if (_checkLastCase && statement instanceof IASTCompoundStatement && statement.getParent() == _switchStatement) {
-				IASTComment comment = getFreestandingComment(statement);
-				/*
-				 * 'comment' is the last comment found in this case (after
-				 * the
-				 * last statement in this "case"
-				 */
-				checkPreviousCase(comment, true);
-			}
-			return super.leave(statement);
-		}
 	}
 
 	/************************************************
@@ -258,9 +199,6 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 		_checkEmptyCase = (Boolean) getPreference(getProblemById(ER_ID, getFile()), PARAM_EMPTY_CASE);
 		_noBreakComment = (String) getPreference(getProblemById(ER_ID, getFile()), PARAM_NO_BREAK_COMMENT);
 		SwitchFindingVisitor visitor = new SwitchFindingVisitor();
-		
 		ast.accept(visitor);
 	}
-
-
 }

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

@@ -20,7 +20,7 @@ import org.eclipse.cdt.codan.internal.checkers.CaseBreakChecker;
 public class CaseBreakCheckerTest extends CheckerTestCase {
 	/*
 	 * (non-Javadoc)
-	 * 
+	 *
 	 * @see org.eclipse.cdt.codan.core.test.CodanTestCase#setUp()
 	 */
 	@Override
@@ -375,7 +375,7 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 	// }
 	public void testNestedSwitches() {
 		loadCodeAndRun(getAboveComment());
-		checkErrorLines(4, 6, 9, 20, 27);
+		checkErrorLines(4, 20, 6, 9, 27);
 	}
 
 	// void foo(void) {
@@ -487,4 +487,37 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		loadCodeAndRun(code);
 		checkNoErrors();
 	}
+
+	// void foo(int a) {
+	//  switch( a ) {
+	//  case 2:
+	//     if (a*2<10)
+	//         return;
+	//     else
+	//         break;
+	//  case 1:
+	//      break;
+	//  }
+	// }
+	public void testIf() {
+		String code = getAboveComment();
+		loadCodeAndRun(code);
+		checkNoErrors();
+	}
+	// void foo(int a) {
+	//  switch( a ) {
+	//  case 2:
+	//     if (a*2<10)
+	//         return;
+	//     else
+	//         a++;
+	//  case 1:
+	//      break;
+	//  }
+	// }
+	public void testIfErr() {
+		String code = getAboveComment();
+		loadCodeAndRun(code);
+		checkErrorLine(3);
+	}
 }