Bug 356037 - "No break at the end of case" warning for a case statement

produced by macro expansion.

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

@@ -52,7 +52,7 @@ problem.name.FormatString = Format String Vulnerability
 checker.name.AssignmentToItself = Assignment to itself
 problem.messagePattern.AssignmentToItself = Assignment to itself ''{0}''
 problem.name.AssignmentToItself = Assignment to itself
-problem.description.AssignmentToItself = Finds expression where left and right side of the assignment is the same, i.e. 'var = var'
+problem.description.AssignmentToItself = Finds expression where left and right sides of the assignment are the same, i.e. 'var = var'
 checker.name.ReturnStyle = Return with parenthesis
 problem.name.ReturnStyle = Return with parenthesis
 problem.messagePattern.ReturnStyle = Return statement has invalid style. Return value should be surrounded by parenthesis
@@ -60,10 +60,10 @@ problem.description.ReturnStyle = Checks for return statements that do no return
 checker.name.SuspiciousSemicolon = Suspicious semicolon
 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);'
+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
-problem.messagePattern.CaseBreak = No break at the end of this case
+problem.messagePattern.CaseBreak = No break at the end of case
 binding.checker.name = Problem Binding Checker
 problem.description.G = Name resolution problem found by the indexer
 problem.messagePattern.G = Symbol ''{0}'' could not be resolved

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

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2010,2011 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
@@ -8,6 +8,7 @@
  * Contributors:
  *     Gil Barash  - Initial implementation
  *     Elena laskavaia - Rewrote checker to reduce false positives in complex cases
+ *     Sergey Prigogin (Google)
  *******************************************************************************/
 package org.eclipse.cdt.codan.internal.checkers;
 
@@ -50,8 +51,7 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 	}
 
 	/**
-	 * This visitor looks for "switch" statements and invokes "SwitchVisitor" on
+	 * This visitor looks for "switch" statements and invokes "SwitchVisitor" on them.
-	 * them.
 	 */
 	class SwitchFindingVisitor extends ASTVisitor {
 		SwitchFindingVisitor() {
@@ -67,7 +67,7 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 		 *         - "continue"
 		 *         - "goto" (does not check that the goto actually exists the
 		 *         switch)
-		 *         - "thorw"
+		 *         - "throw"
 		 *         - "exit"
 		 */
 		protected boolean isBreakOrExitStatement(IASTStatement statement) {
@@ -83,7 +83,7 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 				IASTSwitchStatement switchStmt = (IASTSwitchStatement) statement;
 				IASTStatement body = switchStmt.getBody();
 				if (body instanceof IASTCompoundStatement) {
-					// if not it is not really a switch
+					// If not it is not really a switch
 					IASTStatement[] statements = ((IASTCompoundStatement) body).getStatements();
 					IASTStatement prevCase = null;
 					for (int i = 0; i < statements.length; i++) {
@@ -97,16 +97,16 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 						if (isCaseStatement(curr)) {
 							prevCase = curr;
 						}
-						// next is case or end of switch - means this one is the last
+						// 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
+							// Check that current statement end with break or any other exit statement
 							if (!_checkEmptyCase && isCaseStatement(curr) && next != null) {
-								continue; // empty case & we don't care
+								continue; // Empty case and we don't care
 							}
 							if (!_checkLastCase && next == null) {
-								continue; // last case and we don't care
+								continue; // Last case and we don't care
 							}
-							if (isFallThroughStamement(curr)) {
+							if (!isProducedByMacroExpansion(prevCase) && isFallThroughStamement(curr)) {
 								IASTComment comment = null;
 								if (next != null) {
 									comment = getLeadingComment(next);
@@ -139,7 +139,7 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 		}
 
 		/**
-		 * @param nextstatement
+		 * @param body
 		 * @return
 		 */
 		public boolean isFallThroughStamement(IASTStatement body) {
@@ -171,13 +171,11 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
 		IASTFileLocation astLocation = astNode.getFileLocation();
 		int line = astLocation.getEndingLineNumber();
 		IProblemLocationFactory locFactory = getRuntime().getProblemLocationFactory();
-		return locFactory.createProblemLocation(getFile(), -1,
+		return locFactory.createProblemLocation(getFile(), -1, -1, line);
-				-1, line);
 	}
 
 	/**
-	 * Checks if the given statement is a result of macro expansion with a
+	 * Checks if the given statement is a result of macro expansion with a possible
-	 * possible
 	 * exception for the trailing semicolon.
 	 * 
 	 * @param statement the statement to check.

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

@@ -7,6 +7,7 @@
  *
  * Contributors:
  *     Gil Barash  - Initial implementation
+ *     Sergey Prigogin (Google)
  *******************************************************************************/
 package org.eclipse.cdt.codan.core.internal.checkers;
 
@@ -508,8 +509,9 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		loadCodeAndRun(code);
 		checkNoErrors();
 	}
+
 	// void foo(int a) {
-	//  switch( a ) {
+	//   switch(a) {
 	//   case 2:
 	//     if (a*2<10)
 	//       return;
@@ -525,53 +527,43 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
 		checkErrorLine(7);
 	}
 
-//	#define DEFINE_BREAK {break;}
+	//	#define DEFINE_BREAK {break;}
-//	void foo ( int a )
+	//	void foo(int a) {
-//	{
+	//	  switch (a) {
-//	    switch ( a )
+	//	    case 1:
-//	    {
+	//	      DEFINE_BREAK  // No warning here
-//	        case 1:
+	//	  }
-//	            DEFINE_BREAK  // <-- Warning: No break at the end of this case
+	//	}
-//	    }
-//	}
 	public void testBreakInBraces() {
 		String code = getAboveComment();
 		loadCodeAndRun(code);
 		checkNoErrors();
 	}
 
-
+	//	#define MY_MACRO(i) \
-//	#define MY_MACRO(i)     \
+	//	  case i: {         \
-//	    case i:             \
+	//	  }
-//	    {                   \
+	//
-//	        break;          \
+	//	void f() {
-//	    }
+	//	  int x;
-//
+	//	  switch (x) {
-//	void f()
+	//	    MY_MACRO(1)  // No warning here
-//	{
+	//	  }
-//	    int x;
+	//	}
-//	    switch (x)
-//	    {
-//	        MY_MACRO(1)  // WARNING HERE
-//	    }
-//	}
-
 	public void testInMacro() {
 		String code = getAboveComment();
 		loadCodeAndRun(code);
 		checkNoErrors();
 	}
 
-		//void foo()
+	//  void foo() {
-		//{
+	//    switch (0)
-		//switch(0)
+	//    default: {
-		//default:
+	//    }
-		//{
+	//  }
-		//}
-		//}
 	public void testEmptyCompoundStatement() {
 		String code = getAboveComment();
 		loadCodeAndRun(code);
-			checkErrorLine(6);
+		checkErrorLine(4);
 	}
 }