mirrors/cdt
1
0
Fork 0
mirror of https://github.com/eclipse-cdt/cdt synced 2025-07-21 16:05:25 +02:00
Code Issues Wiki Activity

Bug 329497 - updated check for false negative

Browse source
This commit is contained in:
Alena Laskavaia 2011-02-24 02:55:11 +00:00
parent 2d42d08abc
commit 0989509cda
2 changed files with 102 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

111
codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CaseBreakChecker.java
View file

@ -28,7 +28,8 @@ import org.eclipse.cdt.core.dom.ast.IASTStatement;
import org.eclipse.cdt.core.dom.ast.IASTSwitchStatement; import org.eclipse.cdt.core.dom.ast.IASTSwitchStatement;
import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
public class CaseBreakChecker extends AbstractIndexAstChecker implements ICheckerWithPreferences { public class CaseBreakChecker extends AbstractIndexAstChecker implements
ICheckerWithPreferences {
public static final String ER_ID = "org.eclipse.cdt.codan.internal.checkers.CaseBreakProblem"; //$NON-NLS-1$ public static final String ER_ID = "org.eclipse.cdt.codan.internal.checkers.CaseBreakProblem"; //$NON-NLS-1$
public static final String PARAM_LAST_CASE = "last_case_param"; //$NON-NLS-1$ public static final String PARAM_LAST_CASE = "last_case_param"; //$NON-NLS-1$
public static final String PARAM_EMPTY_CASE = "empty_case_param"; //$NON-NLS-1$ public static final String PARAM_EMPTY_CASE = "empty_case_param"; //$NON-NLS-1$
@ -73,7 +74,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
* @return Is the next comment located after 'node' * @return Is the next comment located after 'node'
*/ */
public boolean isNextAfterThis(IASTNode node) { public boolean isNextAfterThis(IASTNode node) {
return (_comments[_next].getFileLocation().getNodeOffset() > node.getFileLocation().getNodeOffset()); return (_comments[_next].getFileLocation().getNodeOffset() > node
.getFileLocation().getNodeOffset());
} }
/** /**
@ -81,7 +83,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
* @return Is the next comment located after 'node' ends * @return Is the next comment located after 'node' ends
*/ */
public boolean isNextAfterThisEnds(IASTNode node) { public boolean isNextAfterThisEnds(IASTNode node) {
return (_comments[_next].getFileLocation().getNodeOffset() > node.getFileLocation().getNodeOffset() return (_comments[_next].getFileLocation().getNodeOffset() > node
.getFileLocation().getNodeOffset()
+ node.getFileLocation().getNodeLength()); + node.getFileLocation().getNodeLength());
} }
@ -105,11 +108,46 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
_switchStatement = null; _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;
}
/**
* @param statement
* @return true iff the statement is on of:
* - "break" (checks that the break actually exists the "switch")
* - "return"
* - "continue"
* - "goto" (does not check that the goto actually exists the switch)
* - "thorw"
* - "exit"
*/
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);
}
@Override @Override
public int visit(IASTStatement statement) { public int visit(IASTStatement statement) {
if (statement instanceof IASTSwitchStatement) { if (statement instanceof IASTSwitchStatement) {
// Are we already visiting this statement? // Are we already visiting this statement?
if (_switchStatement == null || !statement.equals(_switchStatement)) { if (_switchStatement == null
|| !statement.equals(_switchStatement)) {
SwitchVisitor switch_visitor = new SwitchVisitor(statement); SwitchVisitor switch_visitor = new SwitchVisitor(statement);
statement.accept(switch_visitor); statement.accept(switch_visitor);
return PROCESS_SKIP; return PROCESS_SKIP;
@ -153,7 +191,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
* @return Was a "break" statement the last statement in this case * @return Was a "break" statement the last statement in this case
*/ */
private boolean breakFoundPrevious() { private boolean breakFoundPrevious() {
return _prev_normal_stmnt_offset < _prev_break_stmnt_offset && _prev_case_stmnt_offset < _prev_break_stmnt_offset; return _prev_normal_stmnt_offset < _prev_break_stmnt_offset
&& _prev_case_stmnt_offset < _prev_break_stmnt_offset;
} }
/** /**
@ -193,7 +232,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
@Override @Override
public int visit(IASTStatement statement) { public int visit(IASTStatement statement) {
if (statement instanceof IASTCaseStatement || statement instanceof IASTDefaultStatement) { if (statement instanceof IASTCaseStatement
|| statement instanceof IASTDefaultStatement) {
if (_first_case_statement) { if (_first_case_statement) {
/* /*
* This is the first "case", i.e. the beginning of the * This is the first "case", i.e. the beginning of the
@ -208,7 +248,8 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
*/ */
IASTComment comment = null; IASTComment comment = null;
// Do we have a comment which is before this "case" statement (but after the previous statement)? // Do we have a comment which is before this "case" statement (but after the previous statement)?
while (_commentsIt.hasNext() && !_commentsIt.isNextAfterThis(statement)) { while (_commentsIt.hasNext()
&& !_commentsIt.isNextAfterThis(statement)) {
comment = _commentsIt.getNext(); comment = _commentsIt.getNext();
_commentsIt.advance(); _commentsIt.advance();
} }
@ -219,15 +260,19 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
checkPreviousCase(comment, false); checkPreviousCase(comment, false);
} }
/* Update variables with the new opened "case" */ /* Update variables with the new opened "case" */
_prev_case_stmnt_offset = statement.getFileLocation().getNodeOffset(); _prev_case_stmnt_offset = statement.getFileLocation()
.getNodeOffset();
_prev_case_stmnt = statement; _prev_case_stmnt = statement;
} else if (isBreakOrExitStatement(statement)) { // A "break" statement } else if (isBreakOrExitStatement(statement)) { // A relevant "break" statement
_prev_break_stmnt_offset = statement.getFileLocation().getNodeOffset(); _prev_break_stmnt_offset = statement.getFileLocation()
.getNodeOffset();
} else { // a non-switch related statement } else { // a non-switch related statement
_prev_normal_stmnt_offset = statement.getFileLocation().getNodeOffset(); _prev_normal_stmnt_offset = statement.getFileLocation()
.getNodeOffset();
} }
/* advance comments we already passed */ /* advance comments we already passed */
while (_commentsIt.hasNext() && !_commentsIt.isNextAfterThis(statement)) while (_commentsIt.hasNext()
&& !_commentsIt.isNextAfterThis(statement))
_commentsIt.advance(); _commentsIt.advance();
return super.visit(statement); // This would handle nested "switch"s return super.visit(statement); // This would handle nested "switch"s
} }
@ -238,10 +283,12 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
* Are we leaving the "switch" altogether? (we need to see how the * Are we leaving the "switch" altogether? (we need to see how the
* last "case" ended) * last "case" ended)
*/ */
if (_checkLastCase && statement instanceof IASTCompoundStatement && statement.getParent() == _switchStatement) { if (_checkLastCase && statement instanceof IASTCompoundStatement
&& statement.getParent() == _switchStatement) {
IASTComment comment = null; IASTComment comment = null;
// is "Next" still in the switch's scope? if it is it was after the last statement // is "Next" still in the switch's scope? if it is it was after the last statement
while (_commentsIt.hasNext() && !_commentsIt.isNextAfterThisEnds(statement)) { while (_commentsIt.hasNext()
&& !_commentsIt.isNextAfterThisEnds(statement)) {
comment = _commentsIt.getNext(); comment = _commentsIt.getNext();
_commentsIt.advance(); _commentsIt.advance();
} }
@ -256,34 +303,34 @@ public class CaseBreakChecker extends AbstractIndexAstChecker implements IChecke
} }
/************************************************ /************************************************
* This class's functions... * "CaseBreakChecker" functions...
************************************************/ ************************************************/
public CaseBreakChecker() { public CaseBreakChecker() {
} }
/**
* @param statement
* @return
*/
public boolean isBreakOrExitStatement(IASTStatement statement) {
CxxAstUtils utils = CxxAstUtils.getInstance();
return statement instanceof IASTBreakStatement || statement instanceof IASTReturnStatement
|| statement instanceof IASTContinueStatement || statement instanceof IASTGotoStatement
|| utils.isThrowStatement(statement) || utils.isExitStatement(statement);
}
public void initPreferences(IProblemWorkingCopy problem) { public void initPreferences(IProblemWorkingCopy problem) {
super.initPreferences(problem); super.initPreferences(problem);
addPreference(problem, PARAM_NO_BREAK_COMMENT, CheckersMessages.CaseBreakChecker_DefaultNoBreakCommentDescription, addPreference(
problem,
PARAM_NO_BREAK_COMMENT,
CheckersMessages.CaseBreakChecker_DefaultNoBreakCommentDescription,
DEFAULT_NO_BREAK_COMMENT); DEFAULT_NO_BREAK_COMMENT);
addPreference(problem, PARAM_LAST_CASE, CheckersMessages.CaseBreakChecker_LastCaseDescription, Boolean.TRUE); addPreference(problem, PARAM_LAST_CASE,
addPreference(problem, PARAM_EMPTY_CASE, CheckersMessages.CaseBreakChecker_EmptyCaseDescription, Boolean.FALSE); CheckersMessages.CaseBreakChecker_LastCaseDescription,
Boolean.TRUE);
addPreference(problem, PARAM_EMPTY_CASE,
CheckersMessages.CaseBreakChecker_EmptyCaseDescription,
Boolean.FALSE);
} }
public void processAst(IASTTranslationUnit ast) { public void processAst(IASTTranslationUnit ast) {
_checkLastCase = (Boolean) getPreference(getProblemById(ER_ID, getFile()), PARAM_LAST_CASE); _checkLastCase = (Boolean) getPreference(
_checkEmptyCase = (Boolean) getPreference(getProblemById(ER_ID, getFile()), PARAM_EMPTY_CASE); getProblemById(ER_ID, getFile()), PARAM_LAST_CASE);
_noBreakComment = (String) getPreference(getProblemById(ER_ID, getFile()), PARAM_NO_BREAK_COMMENT); _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(); SwitchFindingVisitor visitor = new SwitchFindingVisitor();
_commentsIt = new CommentsIterator(ast.getComments()); _commentsIt = new CommentsIterator(ast.getComments());
ast.accept(visitor); ast.accept(visitor);

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

@ -117,7 +117,7 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
} }
// void foo(int a) { // void foo(int a) {
// //
// switch( a ) { // switch( a ) {
// case 1: // case 1:
// throw 1; // throw 1;
@ -442,12 +442,31 @@ public class CaseBreakCheckerTest extends CheckerTestCase {
// case 1: // case 1:
// while (a--) // while (a--)
// break; // break;
// case 2:
// while (a--) {
// break;
// }
// } // }
// } // }
public void testEmptyLastCaseWithLoopBreak() { public void testEmptyCaseWithLoopBreak() {
loadCodeAndRun(getAboveComment()); loadCodeAndRun(getAboveComment());
checkNoErrors(); // FALSE NEGATIVE checkErrorLines(3, 6);
//checkErrorLine(3); }
// void foo(int a) {
// switch( a ) {
// case 1: {
// break;
// }
// case 2: {
// a--;
// break;
// }
// }
// }
public void testCaseWithCurlyBrackets() {
loadCodeAndRun(getAboveComment());
checkNoErrors();
} }
// void foo(void) { // void foo(void) {