diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CaseBreakChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CaseBreakChecker.java index 29fe5ff3b64..7a9dd984c01 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CaseBreakChecker.java +++ b/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)) - || statement instanceof IASTReturnStatement || statement instanceof IASTContinueStatement - || statement instanceof IASTGotoStatement || utils.isThrowStatement(statement) || utils.isExitStatement(statement); + return (statement instanceof IASTBreakStatement) || statement instanceof IASTReturnStatement + || statement instanceof IASTContinueStatement || statement instanceof IASTGotoStatement + || utils.isThrowStatement(statement) || utils.isExitStatement(statement); } @Override public int visit(IASTStatement statement) { if (statement instanceof IASTSwitchStatement) { - // Are we already visiting this statement? - if (_switchStatement == null || !statement.equals(_switchStatement)) { - SwitchVisitor switch_visitor = new SwitchVisitor(statement); - statement.accept(switch_visitor); - return PROCESS_SKIP; + IASTSwitchStatement switchStmt = (IASTSwitchStatement) statement; + IASTStatement body = switchStmt.getBody(); + if (body instanceof IASTCompoundStatement) { + // if not it is not really a switch + 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 - * "case" statement (to see that it ends with a "break"). - * Because it extends SwitchFindingVisitor is would also check nested - * "switch"s + * @param comment + * @return */ - class SwitchVisitor extends SwitchFindingVisitor { - private IASTStatement _prev_case_stmnt; - private boolean _first_case_statement; - private int _prev_break_stmnt_offset; - private int _prev_normal_stmnt_offset; - private int _prev_case_stmnt_offset; + 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; + } - SwitchVisitor(IASTStatement switch_statement) { - shouldVisitStatements = true; - _first_case_statement = true; - _switchStatement = switch_statement; - _prev_break_stmnt_offset = 0; - _prev_normal_stmnt_offset = 0; - _prev_case_stmnt_offset = 0; - _prev_case_stmnt = null; - getCommentMap(); - } + /** + * @param statement + * @return + */ + public IASTComment getLeadingComment(IASTStatement statement) { + return getCommentMap().getLastLeadingCommentForNode(statement); + } - /** - * @return Is this an "empty" case (i.e. with no statements in it) - */ - private boolean isEmptyCase() { - return _prev_case_stmnt_offset > _prev_normal_stmnt_offset; - } - - /** - * @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); - } + /** + * @param statement + * @return + */ + public IASTComment getFreestandingComment(IASTStatement statement) { + return getCommentMap().getLastFreestandingCommentForNode(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); } - - } diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/CaseBreakCheckerTest.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/CaseBreakCheckerTest.java index ee4ef45e501..8ada941e0e3 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/CaseBreakCheckerTest.java +++ b/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); + } }