Skip to content

Commit

Permalink
Fix #12431 (Style warnings are reported when only --enable=warning is…
Browse files Browse the repository at this point in the history 
… 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.
  • Loading branch information
@danmar
danmar committed Feb 20, 2024
1 parent ac48167 commit 648f09f
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 31 deletions.
3 changes: 3 additions & 0 deletions lib/checkclass.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2891,6 +2891,9 @@ void CheckClass::virtualFunctionCallInConstructorError(
const std::list<const Token *> & 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;
Expand Down
39 changes: 24 additions & 15 deletions lib/checkother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()) {
Expand All @@ -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,
Expand All @@ -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(),
Expand All @@ -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))
Expand Down Expand Up @@ -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 "
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/testclass.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
30 changes: 15 additions & 15 deletions test/testother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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());
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}

Expand Down

0 comments on commit 648f09f

Please sign in to comment.