From 214423cc4e0b515a1e62b40f254e4750d86f5057 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 4 May 2022 19:33:29 +0200 Subject: [PATCH 1/2] Fix #11041 FN constVariable with array of pointers [regression] --- lib/checkother.cpp | 17 +++++++++-------- test/testother.cpp | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index fccd25ca465..0e23eceff1b 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1543,23 +1543,24 @@ void CheckOther::checkConstPointer() std::set pointers; std::set nonConstPointers; for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { - if (!tok->variable()) + const Variable* const var = tok->variable(); + if (!var) continue; - if (!tok->variable()->isLocal() && !tok->variable()->isArgument()) + if (!var->isLocal() && !var->isArgument()) continue; - const Token* const nameTok = tok->variable()->nameToken(); + const Token* const nameTok = var->nameToken(); // declarations of (static) pointers are (not) split up, array declarations are never split up - if (tok == nameTok && (!tok->variable()->isStatic() || Token::simpleMatch(nameTok->next(), "[")) && + if (tok == nameTok && (!var->isStatic() || Token::simpleMatch(nameTok->next(), "[")) && // range-based for loop !(Token::simpleMatch(nameTok->astParent(), ":") && Token::simpleMatch(nameTok->astParent()->astParent(), "("))) continue; if (!tok->valueType()) continue; - if (tok->valueType()->pointer != 1 || (tok->valueType()->constness & 1) || tok->valueType()->reference != Reference::None) + if (tok->valueType()->pointer != 1 && !(tok->valueType()->pointer == 2 && var->isArray()) || (tok->valueType()->constness & 1) || tok->valueType()->reference != Reference::None) continue; - if (nonConstPointers.find(tok->variable()) != nonConstPointers.end()) + if (nonConstPointers.find(var) != nonConstPointers.end()) continue; - pointers.insert(tok->variable()); + pointers.insert(var); const Token *parent = tok->astParent(); bool deref = false; if (parent && parent->isUnaryOp("*")) @@ -1586,7 +1587,7 @@ void CheckOther::checkConstPointer() else if (Token::simpleMatch(parent, "(") && Token::Match(parent->astOperand1(), "if|while")) continue; } - nonConstPointers.insert(tok->variable()); + nonConstPointers.insert(var); } for (const Variable *p: pointers) { if (p->isArgument()) { diff --git a/test/testother.cpp b/test/testother.cpp index 21da5c39d71..2350b7667cd 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3059,6 +3059,19 @@ class TestOther : public TestFixture { "};\n" "S s;\n"); ASSERT_EQUALS("", errout.str()); + + check("void f(int i) {\n" + " const char *tmp;\n" + " char* a[] = { \"a\", \"aa\" };\n" + " static char* b[] = { \"b\", \"bb\" };\n" + " tmp = a[i];\n" + " printf(\"%s\", tmp);\n" + " tmp = b[i];\n" + " printf(\"%s\", tmp);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'a' can be declared with const\n" + "[test.cpp:4]: (style) Variable 'b' can be declared with const\n", + errout.str()); } void switchRedundantAssignmentTest() { @@ -9213,7 +9226,7 @@ class TestOther : public TestFixture { " int local_argc = 0;\n" " local_argv[local_argc++] = argv[0];\n" "}\n", "test.c"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.c:1]: (style) Parameter 'argv' can be declared with const\n", errout.str()); check("void f() {\n" " int x = 0;\n" From 3494045783eb847fd8cf5a55336b684fac4cbb1f Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Wed, 4 May 2022 23:17:11 +0200 Subject: [PATCH 2/2] Use std::vector for deterministic order of results, use helper variables --- lib/checkother.cpp | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0e23eceff1b..365d5c1bc14 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1540,8 +1540,7 @@ void CheckOther::checkConstPointer() if (!mSettings->severity.isEnabled(Severity::style)) return; - std::set pointers; - std::set nonConstPointers; + std::vector pointers, nonConstPointers; for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { const Variable* const var = tok->variable(); if (!var) @@ -1554,32 +1553,34 @@ void CheckOther::checkConstPointer() // range-based for loop !(Token::simpleMatch(nameTok->astParent(), ":") && Token::simpleMatch(nameTok->astParent()->astParent(), "("))) continue; - if (!tok->valueType()) + const ValueType* const vt = tok->valueType(); + if (!vt) continue; - if (tok->valueType()->pointer != 1 && !(tok->valueType()->pointer == 2 && var->isArray()) || (tok->valueType()->constness & 1) || tok->valueType()->reference != Reference::None) + if (vt->pointer != 1 && !(vt->pointer == 2 && var->isArray()) || (vt->constness & 1) || vt->reference != Reference::None) continue; - if (nonConstPointers.find(var) != nonConstPointers.end()) + if (std::find(nonConstPointers.begin(), nonConstPointers.end(), var) != nonConstPointers.end()) continue; - pointers.insert(var); - const Token *parent = tok->astParent(); + pointers.emplace_back(var); + const Token* const parent = tok->astParent(); bool deref = false; if (parent && parent->isUnaryOp("*")) deref = true; - else if (Token::simpleMatch(parent, "[") && parent->astOperand1() == tok && parent->astOperand1() != nameTok) + else if (Token::simpleMatch(parent, "[") && parent->astOperand1() == tok && tok != nameTok) deref = true; if (deref) { - if (Token::Match(parent->astParent(), "%cop%") && !parent->astParent()->isUnaryOp("&") && !parent->astParent()->isUnaryOp("*")) + const Token* const gparent = parent->astParent(); + if (Token::Match(gparent, "%cop%") && !gparent->isUnaryOp("&") && !gparent->isUnaryOp("*")) continue; - if (Token::simpleMatch(parent->astParent(), "return")) + if (Token::simpleMatch(gparent, "return")) continue; - else if (Token::Match(parent->astParent(), "%assign%") && parent == parent->astParent()->astOperand2()) { + else if (Token::Match(gparent, "%assign%") && parent == gparent->astOperand2()) { bool takingRef = false; - const Token *lhs = parent->astParent()->astOperand1(); + const Token *lhs = gparent->astOperand1(); if (lhs && lhs->variable() && lhs->variable()->isReference() && lhs->variable()->nameToken() == lhs) takingRef = true; if (!takingRef) continue; - } else if (Token::simpleMatch(parent->astParent(), "[") && parent->astParent()->astOperand2() == parent) + } else if (Token::simpleMatch(gparent, "[") && gparent->astOperand2() == parent) continue; } else { if (Token::Match(parent, "%oror%|%comp%|&&|?|!|-")) @@ -1587,14 +1588,14 @@ void CheckOther::checkConstPointer() else if (Token::simpleMatch(parent, "(") && Token::Match(parent->astOperand1(), "if|while")) continue; } - nonConstPointers.insert(var); + nonConstPointers.emplace_back(var); } for (const Variable *p: pointers) { if (p->isArgument()) { if (!p->scope() || !p->scope()->function || p->scope()->function->isImplicitlyVirtual(true) || p->scope()->function->hasVirtualSpecifier()) continue; } - if (nonConstPointers.find(p) == nonConstPointers.end()) { + if (std::find(nonConstPointers.begin(), nonConstPointers.end(), p) == nonConstPointers.end()) { const Token *start = (p->isArgument()) ? p->scope()->bodyStart : p->nameToken()->next(); const int indirect = p->isArray() ? p->dimensions().size() : 1; if (isVariableChanged(start, p->scope()->bodyEnd, indirect, p->declarationId(), false, mSettings, mTokenizer->isCPP()))