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 2d79a703881..92c33fd3abe 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 @@ -193,3 +193,7 @@ problem.name.MultipleDeclarationsProblem = Multiple variable declaration problem.messagePattern.MultipleDeclarationsProblem = Variables should be declared one per line to improve readability problem.description.MultipleDeclarationsProblem = This rule will flag multiple declarations on one line +checker.name.FloatCompareChecker = Float compare checker +problem.name.FloatCompareProblem = Direct float comparison +problem.messagePattern.FloatCompareProblem = Float numbers should be compared using an epsilon +problem.description.FloatCompareProblem = This rule will flag direct float or double comparison diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index 04c37c179a4..9a1a0ab2c83 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -507,6 +507,21 @@ messagePattern="%problem.messagePattern.CopyrightProblem" name="%problem.name.CopyrightProblem"> + + + + fields, IBinding binding, IIndex index) { for (IField field : fields) { - if (areEquivalentBindings(binding, field, index)) { + if (CPPVisitor.areEquivalentBindings(binding, field, index)) { return field; } } @@ -232,26 +232,6 @@ public class ClassMembersInitializationChecker extends AbstractIndexAstChecker { return null; } - private boolean areEquivalentBindings(IBinding binding1, IBinding binding2, IIndex index) { - if (binding1.equals(binding2)) { - return true; - } - if ((binding1 instanceof IIndexBinding) != (binding2 instanceof IIndexBinding) && index != null) { - if (binding1 instanceof IIndexBinding) { - binding2 = index.adaptBinding(binding2); - } else { - binding1 = index.adaptBinding(binding1); - } - if (binding1 == null || binding2 == null) { - return false; - } - if (binding1.equals(binding2)) { - return true; - } - } - return false; - } - /** Checks whether class member of the specified type should be initialized * * @param type Type to check diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/FloatCompareChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/FloatCompareChecker.java new file mode 100644 index 00000000000..10dd018b0cb --- /dev/null +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/FloatCompareChecker.java @@ -0,0 +1,153 @@ +/******************************************************************************* + * Copyright (c) 2019 Marco Stornelli + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.cdt.codan.internal.checkers; + +import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker; +import org.eclipse.cdt.core.dom.ast.ASTVisitor; +import org.eclipse.cdt.core.dom.ast.IASTBinaryExpression; +import org.eclipse.cdt.core.dom.ast.IASTExpression; +import org.eclipse.cdt.core.dom.ast.IASTFieldReference; +import org.eclipse.cdt.core.dom.ast.IASTIdExpression; +import org.eclipse.cdt.core.dom.ast.IASTLiteralExpression; +import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; +import org.eclipse.cdt.core.dom.ast.IBasicType; +import org.eclipse.cdt.core.dom.ast.IBinding; +import org.eclipse.cdt.core.dom.ast.IType; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTExpression; +import org.eclipse.cdt.internal.core.dom.parser.ValueFactory; +import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor; +import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil; + +@SuppressWarnings("restriction") +public class FloatCompareChecker extends AbstractIndexAstChecker { + public static final String ERR_ID = "org.eclipse.cdt.codan.internal.checkers.FloatCompareProblem"; //$NON-NLS-1$ + + @Override + public void processAst(IASTTranslationUnit ast) { + ast.accept(new ASTVisitor() { + { + shouldVisitExpressions = true; + } + + private boolean equals(IASTExpression expr1, IASTExpression expr2) { + if (expr1 instanceof ICPPASTExpression && expr2 instanceof ICPPASTExpression) { + return ((ICPPASTExpression) expr1).getEvaluation() + .isEquivalentTo(((ICPPASTExpression) expr2).getEvaluation()); + } + if (expr1 instanceof IASTIdExpression && expr2 instanceof IASTIdExpression) { + IBinding leftLeftBinding = ((IASTIdExpression) expr1).getName().resolveBinding(); + IBinding rightLeftBinding = ((IASTIdExpression) expr2).getName().resolveBinding(); + if (CPPVisitor.areEquivalentBindings(leftLeftBinding, rightLeftBinding, + expr1.getTranslationUnit().getIndex())) { + return true; + } + } else if (expr1 instanceof IASTLiteralExpression && expr2 instanceof IASTLiteralExpression) { + Number n1 = ValueFactory.getConstantNumericalValue(expr1); + Number n2 = ValueFactory.getConstantNumericalValue(expr2); + if (n1 != null && n1.equals(n2)) + return true; + } else if (expr1 instanceof IASTFieldReference && expr2 instanceof IASTFieldReference) { + IBinding leftLeftBinding = ((IASTFieldReference) expr1).getFieldName().resolveBinding(); + IBinding rightLeftBinding = ((IASTFieldReference) expr2).getFieldName().resolveBinding(); + if (CPPVisitor.areEquivalentBindings(leftLeftBinding, rightLeftBinding, + expr1.getTranslationUnit().getIndex())) { + return true; + } + } + return false; + } + + private boolean processDirect(IASTBinaryExpression expression) { + IASTBinaryExpression binary = expression; + if ((binary.getOperator() == IASTBinaryExpression.op_notequals + || binary.getOperator() == IASTBinaryExpression.op_equals) + && (isFloat(binary.getOperand1().getExpressionType()) + || isFloat(binary.getOperand2().getExpressionType()))) { + reportProblem(ERR_ID, expression); + return true; + } + return false; + } + + private boolean processIndirect(IASTBinaryExpression binary) { + if (binary.getOperator() == IASTBinaryExpression.op_logicalAnd) { + return processIndirect(binary, false); + } else if (binary.getOperator() == IASTBinaryExpression.op_logicalOr) { + return processIndirect(binary, true); + } + return false; + } + + private boolean processIndirect(IASTBinaryExpression expression, boolean invert) { + int cond1Test = IASTBinaryExpression.op_lessEqual; + int cond2Test = IASTBinaryExpression.op_greaterEqual; + + if (invert) { + cond1Test = IASTBinaryExpression.op_lessThan; + cond2Test = IASTBinaryExpression.op_greaterThan; + } + + IASTBinaryExpression binary = expression; + IASTExpression left = binary.getOperand1(); + IASTExpression right = binary.getOperand2(); + if (left instanceof IASTBinaryExpression && right instanceof IASTBinaryExpression) { + int leftOp = ((IASTBinaryExpression) left).getOperator(); + int rightOp = ((IASTBinaryExpression) right).getOperator(); + if ((leftOp == cond1Test && rightOp == cond2Test) + || (rightOp == cond1Test && leftOp == cond2Test)) { + //Case 1: + // a <= b && c >= d + // Ex. f <= 3.14 && f >= 3.14 + //Case 2: + // a >= b && c <= d + // Ex. f >= 3.14 && f <= 3.14 + IASTExpression leftLeft = ((IASTBinaryExpression) left).getOperand1(); + IASTExpression leftRight = ((IASTBinaryExpression) left).getOperand2(); + IASTExpression rightLeft = ((IASTBinaryExpression) right).getOperand1(); + IASTExpression rightRight = ((IASTBinaryExpression) right).getOperand2(); + if (equals(leftLeft, rightLeft) && equals(leftRight, rightRight) + && (isFloat(leftLeft.getExpressionType()) || isFloat(leftRight.getExpressionType()))) { + reportProblem(ERR_ID, expression); + return true; + } + } + } + return false; + } + + @Override + public int visit(IASTExpression expression) { + if (expression instanceof IASTBinaryExpression) { + boolean res = processDirect((IASTBinaryExpression) expression); + if (!res) + res = processIndirect((IASTBinaryExpression) expression); + } + return PROCESS_CONTINUE; + } + }); + } + + private boolean isFloat(IType type) { + type = SemanticUtil.getNestedType(type, SemanticUtil.REF | SemanticUtil.TDEF | SemanticUtil.ALLCVQ); + if (!(type instanceof IBasicType)) { + return false; + } + IBasicType.Kind k = ((IBasicType) type).getKind(); + switch (k) { + case eFloat: + case eDouble: + case eFloat128: + return true; + default: + return false; + } + } +} diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/FloatCompareCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/FloatCompareCheckerTest.java new file mode 100644 index 00000000000..c2669d21a5a --- /dev/null +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/FloatCompareCheckerTest.java @@ -0,0 +1,221 @@ +/******************************************************************************* + * Copyright (c) 2019 Marco Stornelli + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.cdt.codan.core.internal.checkers; + +import org.eclipse.cdt.codan.core.tests.CheckerTestCase; +import org.eclipse.cdt.codan.internal.checkers.FloatCompareChecker; + +/** + * Test for {@link FloatCompareChecker} class + */ +public class FloatCompareCheckerTest extends CheckerTestCase { + + public static final String ERR_ID = FloatCompareChecker.ERR_ID; + + @Override + public void setUp() throws Exception { + super.setUp(); + enableProblems(ERR_ID); + } + + @Override + public boolean isCpp() { + return true; + } + + @Override + public boolean isHeader() { + return true; + } + + //int main() { + // float a; + // float b; + // if (a == b) + // return 1; + // return 0; + //} + public void testEqualWithFloat() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //int main() { + // float a; + // float b; + // if (a != b) + // return 1; + // return 0; + //} + public void testDifferentWithFloat() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //int main() { + // double a; + // double b; + // if (a == b) + // return 1; + // return 0; + //} + public void testWithDouble() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //int main() { + // double a; + // float b; + // if (a == b) + // return 1; + // return 0; + //} + public void testWithMixedTypes() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //typedef float myType; + //int main() { + // myType a; + // myType b; + // if (a == b) + // return 1; + // return 0; + //} + public void testWithTypedef() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ERR_ID); + } + + //int main() { + // float a1; + // float b1; + // const float& a = a1; + // const float& b = b1; + // if (a == b) + // return 1; + // return 0; + //} + public void testWithQualififedTypes() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ERR_ID); + } + + //int main() { + // float a1; + // float b1; + // float* a = &a1; + // float* b = &b1; + // if (*a == *b) + // return 1; + // return 0; + //} + public void testWithDerefPointers() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ERR_ID); + } + + //int main() { + // float a1; + // float b1; + // float* a = &a1; + // float* b = &b1; + // if (a == b) + // return 1; + // return 0; + //} + public void testWithPointers() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } + + //int foo() { + // float a1; + // float b1; + // return a1 == b1 ? 0 : 1; + //} + public void testWithTernaryOperator() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //int foo() { + // float a1; + // if (a1 <= 3.14 && a1 >= 3.14) + // a1++; + //} + public void testWithIndirectEqual1() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (a1 >= 3.14 && a1 <= 3.14) + // a1++; + //} + public void testWithIndirectEqual2() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (!(a1 != 2)) + // a1++; + //} + public void testWithIndirectEqual3() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (!(a1 < 3.14 || a1 > 3.14)) + // a1++; + //} + public void testWithIndirectEqual4() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (a1 < 3.14 || a1 > 3.14) + // a1++; + //} + public void testWithIndirectNotEqual1() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (a1 > 3.14 || a1 < 3.14) + // a1++; + //} + public void testWithIndirectNotEqual2() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int foo() { + // float a1; + // if (a1 < 3.14 || a1 > 3.15) + // a1++; + //} + public void testWithUpperLowerBounds() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } +} diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java index 7a4b4b25e04..35b3f3237b4 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java @@ -26,6 +26,7 @@ import org.eclipse.cdt.codan.core.internal.checkers.CommentCheckerLineTests; import org.eclipse.cdt.codan.core.internal.checkers.CommentCheckerNestedTests; import org.eclipse.cdt.codan.core.internal.checkers.CopyrightCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.DecltypeAutoCheckerTest; +import org.eclipse.cdt.codan.core.internal.checkers.FloatCompareCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.FormatStringCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.GotoStatementCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.NonVirtualDestructorCheckerTest; @@ -37,6 +38,7 @@ import org.eclipse.cdt.codan.core.internal.checkers.SuggestedParenthesisCheckerT import org.eclipse.cdt.codan.core.internal.checkers.SuspiciousSemicolonCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.SwitchCaseCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.UnusedSymbolInFileScopeCheckerTest; +import org.eclipse.cdt.codan.core.internal.checkers.UsingInHeaderCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.VariablesCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.VirtualMethodCallCheckerTest; import org.eclipse.cdt.codan.internal.checkers.ui.quickfix.AssignmentInConditionQuickFixTest; @@ -99,6 +101,8 @@ public class AutomatedIntegrationSuite extends TestSuite { suite.addTestSuite(VirtualMethodCallCheckerTest.class); suite.addTestSuite(AssignmentOperatorCheckerTest.class); suite.addTestSuite(VariablesCheckerTest.class); + suite.addTestSuite(UsingInHeaderCheckerTest.class); + suite.addTestSuite(FloatCompareCheckerTest.class); // framework suite.addTest(CodanFastTestSuite.suite()); // quick fixes diff --git a/core/org.eclipse.cdt.core/META-INF/MANIFEST.MF b/core/org.eclipse.cdt.core/META-INF/MANIFEST.MF index dd68713e2c6..97d1389faaa 100644 --- a/core/org.eclipse.cdt.core/META-INF/MANIFEST.MF +++ b/core/org.eclipse.cdt.core/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.cdt.core; singleton:=true -Bundle-Version: 6.9.0.qualifier +Bundle-Version: 6.10.0.qualifier Bundle-Activator: org.eclipse.cdt.core.CCorePlugin Bundle-Vendor: %providerName Bundle-Localization: plugin diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPVisitor.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPVisitor.java index 37f1353c852..11cc4769f92 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPVisitor.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPVisitor.java @@ -1733,7 +1733,7 @@ public class CPPVisitor extends ASTQueries { return areEquivalentBindings(candidate, target, index); } - private static boolean areEquivalentBindings(IBinding binding1, IBinding binding2, IIndex index) { + public static boolean areEquivalentBindings(IBinding binding1, IBinding binding2, IIndex index) { if (binding1.equals(binding2)) { return true; }