From 648f09f5d4473b705596ae97f0b9af83e9205283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 20 Feb 2024 17:35:45 +0100 Subject: [PATCH] Fix #12431 (Style warnings are reported when only --enable=warning is used) (#5974) * do not show knownConditionTrueFalse, virtualCallInConstructor, duplicateExpression style messages unless --enable=style is configured. * change selfAssignment from warning to style => it's not about undefined behavior * don't claim that code such as `a = b|c;` is a condition. --- lib/checkclass.cpp | 3 +++ lib/checkother.cpp | 39 ++++++++++++++++++++++++--------------- test/testclass.cpp | 2 +- test/testother.cpp | 30 +++++++++++++++--------------- 4 files changed, 43 insertions(+), 31 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 4ba5c964951..08e75aef738 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2891,6 +2891,9 @@ void CheckClass::virtualFunctionCallInConstructorError( const std::list & tokStack, const std::string &funcname) { + if (scopeFunction && !mSettings->severity.isEnabled(Severity::style) && !mSettings->isPremiumEnabled("virtualCallInConstructor")) + return; + const char * scopeFunctionTypeName = scopeFunction ? getFunctionTypeName(scopeFunction->type) : "constructor"; ErrorPath errorPath; diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 8663d7f2237..4438338b7bc 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2402,10 +2402,19 @@ namespace { void CheckOther::checkDuplicateExpression() { - const bool styleEnabled = mSettings->severity.isEnabled(Severity::style); - const bool warningEnabled = mSettings->severity.isEnabled(Severity::warning); - if (!styleEnabled && !warningEnabled) - return; + { + const bool styleEnabled = mSettings->severity.isEnabled(Severity::style); + const bool premiumEnabled = mSettings->isPremiumEnabled("oppositeExpression") || + mSettings->isPremiumEnabled("duplicateExpression") || + mSettings->isPremiumEnabled("duplicateAssignExpression") || + mSettings->isPremiumEnabled("duplicateExpressionTernary") || + mSettings->isPremiumEnabled("duplicateValueTernary") || + mSettings->isPremiumEnabled("selfAssignment") || + mSettings->isPremiumEnabled("knownConditionTrueFalse"); + + if (!styleEnabled && !premiumEnabled) + return; + } logChecker("CheckOther::checkDuplicateExpression"); // style,warning @@ -2511,9 +2520,9 @@ void CheckOther::checkDuplicateExpression() !findExpressionChanged(tok, tok, loopTok->link()->next()->link(), mSettings, cpp)) { const bool isEnum = tok->scope()->type == Scope::eEnum; const bool assignment = !isEnum && tok->str() == "="; - if (assignment && warningEnabled) + if (assignment) selfAssignmentError(tok, tok->astOperand1()->expressionString()); - else if (styleEnabled && !isEnum) { + else if (!isEnum) { if (cpp && mSettings->standards.cpp >= Standards::CPP11 && tok->str() == "==") { const Token* parent = tok->astParent(); while (parent && parent->astParent()) { @@ -2535,11 +2544,10 @@ void CheckOther::checkDuplicateExpression() mSettings->library, true, false)) { - if (warningEnabled && isWithoutSideEffects(cpp, tok->astOperand1())) { + if (isWithoutSideEffects(cpp, tok->astOperand1())) { selfAssignmentError(tok, tok->astOperand1()->expressionString()); } - } else if (styleEnabled && - isOppositeExpression(cpp, + } else if (isOppositeExpression(cpp, tok->astOperand1(), tok->astOperand2(), mSettings->library, @@ -2550,7 +2558,7 @@ void CheckOther::checkDuplicateExpression() isWithoutSideEffects(cpp, tok->astOperand1())) { oppositeExpressionError(tok, std::move(errorPath)); } else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative - if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() && + if (tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(cpp, true, tok->astOperand2(), @@ -2577,7 +2585,7 @@ void CheckOther::checkDuplicateExpression() } } } - } else if (styleEnabled && tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") { + } else if (tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") { if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) && !isVariableChanged(tok->astParent(), /*indirect*/ 0, mSettings, cpp) && isConstStatement(tok->astOperand1(), cpp) && isConstStatement(tok->astOperand2(), cpp)) @@ -2611,17 +2619,18 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string& op = opTok ? opTok->str() : "&&"; std::string msg = "Same expression " + (hasMultipleExpr ? "\'" + expr1 + "\'" + " found multiple times in chain of \'" + op + "\' operators" : "on both sides of \'" + op + "\'"); const char *id = "duplicateExpression"; - if (expr1 != expr2 && (!opTok || !opTok->isArithmeticalOp())) { + if (expr1 != expr2 && (!opTok || Token::Match(opTok, "%oror%|%comp%|&&|?|!"))) { id = "knownConditionTrueFalse"; std::string exprMsg = "The comparison \'" + expr1 + " " + op + " " + expr2 + "\' is always "; if (Token::Match(opTok, "==|>=|<=")) msg = exprMsg + "true"; else if (Token::Match(opTok, "!=|>|<")) msg = exprMsg + "false"; - if (!Token::Match(tok1, "%num%|NULL|nullptr") && !Token::Match(tok2, "%num%|NULL|nullptr")) - msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value"; } + if (expr1 != expr2 && !Token::Match(tok1, "%num%|NULL|nullptr") && !Token::Match(tok2, "%num%|NULL|nullptr")) + msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value"; + reportError(errors, Severity::style, id, msg + (std::string(".\nFinding the same expression ") + (hasMultipleExpr ? "more than once in a condition" : "on both sides of an operator")) + " is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to " @@ -2659,7 +2668,7 @@ void CheckOther::duplicateValueTernaryError(const Token *tok) void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname) { - reportError(tok, Severity::warning, + reportError(tok, Severity::style, "selfAssignment", "$symbol:" + varname + "\n" "Redundant assignment of '$symbol' to itself.", CWE398, Certainty::normal); diff --git a/test/testclass.cpp b/test/testclass.cpp index 8d6fb0478cb..6c26bb24add 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8027,7 +8027,7 @@ class TestClass : public TestFixture { errout.str(""); // Check.. - const Settings settings = settingsBuilder().severity(Severity::warning).certainty(Certainty::inconclusive, inconclusive).build(); + const Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::style).certainty(Certainty::inconclusive, inconclusive).build(); Preprocessor preprocessor(settings); diff --git a/test/testother.cpp b/test/testother.cpp index 3f938af2e85..fd0bd743878 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -5467,26 +5467,26 @@ class TestOther : public TestFixture { " x = x;\n" " return 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'x' to itself.\n", errout.str()); check("void foo()\n" "{\n" " int x = x;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'x' to itself.\n", errout.str()); check("struct A { int b; };\n" "void foo(A* a1, A* a2) {\n" " a1->b = a1->b;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'a1->b' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'a1->b' to itself.\n", errout.str()); check("int x;\n" "void f()\n" "{\n" " x = x = 3;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'x' to itself.\n", errout.str()); // #4073 (segmentation fault) check("void Foo::myFunc( int a )\n" @@ -5514,7 +5514,7 @@ class TestOther : public TestFixture { " BAR *x = getx();\n" " x = x;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'x' to itself.\n", errout.str()); // #2502 - non-primitive type -> there might be some side effects check("void foo()\n" @@ -5546,7 +5546,7 @@ class TestOther : public TestFixture { "void f() {\n" " i = i;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'i' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'i' to itself.\n", errout.str()); // #4291 - id for variables accessed through 'this' check("class Foo {\n" @@ -5556,7 +5556,7 @@ class TestOther : public TestFixture { "void Foo::func() {\n" " this->var = var;\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'this->var' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (style) Redundant assignment of 'this->var' to itself.\n", errout.str()); check("class Foo {\n" " int var;\n" @@ -5575,7 +5575,7 @@ class TestOther : public TestFixture { "void f() {\n" " struct callbacks ops = { .s = ops.s };\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'something' to itself.\n", "", errout.str()); + TODO_ASSERT_EQUALS("[test.cpp:6]: (style) Redundant assignment of 'something' to itself.\n", "", errout.str()); check("class V\n" "{\n" @@ -5590,9 +5590,9 @@ class TestOther : public TestFixture { " }\n" " double x, y, z;\n" "};"); - ASSERT_EQUALS("[test.cpp:10]: (warning) Redundant assignment of 'x' to itself.\n" - "[test.cpp:10]: (warning) Redundant assignment of 'y' to itself.\n" - "[test.cpp:10]: (warning) Redundant assignment of 'z' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:10]: (style) Redundant assignment of 'x' to itself.\n" + "[test.cpp:10]: (style) Redundant assignment of 'y' to itself.\n" + "[test.cpp:10]: (style) Redundant assignment of 'z' to itself.\n", errout.str()); check("void f(int i) { i = !!i; }"); ASSERT_EQUALS("", errout.str()); @@ -5602,7 +5602,7 @@ class TestOther : public TestFixture { " int &ref = x;\n" " ref = x;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'ref' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'ref' to itself.\n", errout.str()); check("class Foo {\n" // #9850 " int i{};\n" @@ -7057,7 +7057,7 @@ class TestOther : public TestFixture { " int var = buffer[index - 1];\n" " return buffer[index - 1] - var;\n" // << "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '-'.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '-' because 'buffer[index-1]' and 'var' represent the same value.\n", errout.str()); } void duplicateExpression13() { //#7899 @@ -10750,8 +10750,8 @@ class TestOther : public TestFixture { " int x = x = y + 1;\n" "}", "test.c"); ASSERT_EQUALS( - "[test.c:2]: (warning) Redundant assignment of 'x' to itself.\n" - "[test.c:2]: (warning) Redundant assignment of 'x' to itself.\n", // duplicate + "[test.c:2]: (style) Redundant assignment of 'x' to itself.\n" + "[test.c:2]: (style) Redundant assignment of 'x' to itself.\n", // duplicate errout.str()); }