From db436ae417cf0d309dacd3f08aee37534b8f7422 Mon Sep 17 00:00:00 2001 From: Alena Laskavaia Date: Tue, 18 Jan 2011 02:55:37 +0000 Subject: [PATCH] Bug 333813 - fixed fp, default settings and description --- .../OSGI-INF/l10n/bundle.properties | 2 +- .../internal/checkers/CaseBreakChecker.java | 75 +++++++++++-------- .../internal/checkers/messages.properties | 2 +- .../checkers/CaseBreakCheckerTest.java | 38 +++++++++- .../cdt/codan/core/test/CheckerTestCase.java | 50 ++++++++----- 5 files changed, 113 insertions(+), 54 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties index d7bf563eb14..8fb111c7fdd 100644 --- a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties +++ b/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 \ No newline at end of file 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 7f89ca9f1fc..5a00dbff76d 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 @@ -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) { diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/messages.properties b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/messages.properties index 849783659f1..bd30a1fcd99 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/messages.properties +++ b/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 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 ad4ed663794..09dda958973 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 @@ -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(); + } } diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CheckerTestCase.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CheckerTestCase.java index dca61786276..45f17f4495c 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CheckerTestCase.java +++ b/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)); + } } /**