Skip to content

Commit 8e2fdf8

Browse files
firewavefirewave
committed
bail out when adding a suppression more than once / added Suppressions::updateSuppressionState() 
1 parent 524e979 commit 8e2fdf8

10 files changed

Lines changed: 246 additions & 11 deletions

File tree

‎gui/mainwindow.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1026,7 +1026,7 @@ QPair<bool,Settings> MainWindow::getCppcheckSettings()
10261026
}
10271027

10281028
for (const SuppressionList::Suppression &suppression : mProjectFile->getCheckingSuppressions()) {
1029-
result.supprs.nomsg.addSuppression(suppression);
1029+
result.supprs.nomsg.addSuppression(suppression); // TODO: check result
10301030
}
10311031

10321032
// Only check the given -D configuration

‎lib/cppcheck.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,12 @@ unsigned int CppCheck::check(const FileSettings &fs)
600600
return returnValue;
601601
}
602602
const unsigned int returnValue = temp.checkFile(Path::simplifyPath(fs.filename), fs.cfg);
603-
mSettings.supprs.nomsg.addSuppressions(temp.mSettings.supprs.nomsg.getSuppressions());
603+
for (const auto& suppr : temp.mSettings.supprs.nomsg.getSuppressions())
604+
{
605+
const bool res = mSettings.supprs.nomsg.updateSuppressionState(suppr);
606+
if (!res)
607+
throw InternalError(nullptr, "could not update suppression '" + suppr.errorId + "'"); // TODO: remove
608+
}
604609
if (mUnusedFunctionsCheck)
605610
mUnusedFunctionsCheck->updateFunctionData(*temp.mUnusedFunctionsCheck);
606611
return returnValue;

‎lib/importproject.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1263,7 +1263,7 @@ bool ImportProject::importCppcheckGuiProject(std::istream &istr, Settings *setti
12631263

12641264
for (const std::string &p : paths)
12651265
guiProject.pathNames.push_back(p);
1266-
settings->supprs.nomsg.addSuppressions(std::move(suppressions));
1266+
settings->supprs.nomsg.addSuppressions(std::move(suppressions)); // TODO: check result
12671267
settings->checkHeaders = temp.checkHeaders;
12681268
settings->checkUnusedTemplates = temp.checkUnusedTemplates;
12691269
settings->maxCtuDepth = temp.maxCtuDepth;

‎lib/preprocessor.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ static void addInlineSuppressions(const simplecpp::TokenList &tokens, const Sett
253253
suppr.lineNumber = supprBegin->lineNumber;
254254
suppr.type = SuppressionList::Type::block;
255255
inlineSuppressionsBlockBegin.erase(supprBegin);
256-
suppressions.addSuppression(std::move(suppr));
256+
suppressions.addSuppression(std::move(suppr)); // TODO: check result
257257
throwError = false;
258258
break;
259259
}
@@ -278,10 +278,10 @@ static void addInlineSuppressions(const simplecpp::TokenList &tokens, const Sett
278278
suppr.thisAndNextLine = thisAndNextLine;
279279
suppr.lineNumber = tok->location.line;
280280
suppr.macroName = macroName;
281-
suppressions.addSuppression(std::move(suppr));
281+
suppressions.addSuppression(std::move(suppr)); // TODO: check result
282282
} else if (SuppressionList::Type::file == suppr.type) {
283283
if (onlyComments)
284-
suppressions.addSuppression(std::move(suppr));
284+
suppressions.addSuppression(std::move(suppr)); // TODO: check result
285285
else
286286
bad.emplace_back(suppr.fileName, suppr.lineNumber, "File suppression should be at the top of the file");
287287
}

‎lib/suppressions.cpp‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -250,10 +250,7 @@ std::string SuppressionList::addSuppression(SuppressionList::Suppression suppres
250250
auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(),
251251
std::bind(&Suppression::isSameParameters, &suppression, std::placeholders::_1));
252252
if (foundSuppression != mSuppressions.end()) {
253-
// Update matched state of existing global suppression
254-
if (!suppression.isLocal() && suppression.matched)
255-
foundSuppression->matched = suppression.matched;
256-
return "";
253+
return "suppression '" + suppression.toString() + "' already exists";
257254
}
258255

259256
// Check that errorId is valid..
@@ -289,6 +286,21 @@ std::string SuppressionList::addSuppressions(std::list<Suppression> suppressions
289286
return "";
290287
}
291288

289+
bool SuppressionList::updateSuppressionState(const SuppressionList::Suppression& suppression)
290+
{
291+
// Check if suppression is already in list
292+
auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(),
293+
std::bind(&Suppression::isSameParameters, &suppression, std::placeholders::_1));
294+
if (foundSuppression != mSuppressions.end()) {
295+
// Update matched state of existing global suppression
296+
if (!suppression.isLocal() && suppression.matched)
297+
foundSuppression->matched = suppression.matched;
298+
return true;
299+
}
300+
301+
return false;
302+
}
303+
292304
void SuppressionList::ErrorMessage::setFileName(std::string s)
293305
{
294306
mFileName = Path::simplifyPath(std::move(s));
@@ -577,3 +589,22 @@ bool SuppressionList::reportUnmatchedSuppressions(const std::list<SuppressionLis
577589
}
578590
return err;
579591
}
592+
593+
std::string SuppressionList::Suppression::toString() const
594+
{
595+
std::string s;
596+
s+= errorId;
597+
if (!fileName.empty()) {
598+
s += ':';
599+
s += fileName;
600+
if (lineNumber != -1) {
601+
s += ':';
602+
s += std::to_string(lineNumber);
603+
}
604+
}
605+
if (!symbolName.empty()) {
606+
s += ':';
607+
s += symbolName;
608+
}
609+
return s;
610+
}

‎lib/suppressions.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,8 @@ class CPPCHECKLIB SuppressionList {
134134
thisAndNextLine == other.thisAndNextLine;
135135
}
136136

137+
std::string toString() const;
138+
137139
std::string errorId;
138140
std::string fileName;
139141
int lineNumber = NO_LINE;
@@ -194,6 +196,13 @@ class CPPCHECKLIB SuppressionList {
194196
*/
195197
std::string addSuppressions(std::list<Suppression> suppressions);
196198

199+
/**
200+
* @brief Updates the state of the given suppression.
201+
* @param suppression the suppression to update
202+
* @return true if suppression to update was found
203+
*/
204+
bool updateSuppressionState(const SuppressionList::Suppression& suppression);
205+
197206
/**
198207
* @brief Returns true if this message should not be shown to the user.
199208
* @param errmsg error message

‎test/cli/inline-suppress_test.py‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ def __test_compile_commands_unused_function_suppression(tmpdir, use_j):
172172
assert ret == 0, stdout
173173

174174

175+
@pytest.mark.xfail(strict=True) # TODO: need to propagate back inline suppressions
175176
def test_compile_commands_unused_function_suppression(tmpdir):
176177
__test_compile_commands_unused_function_suppression(tmpdir, False)
177178

@@ -282,3 +283,63 @@ def test_suppress_unmatched_inline_suppression(): # 11172
282283
assert lines == []
283284
assert stdout == ''
284285
assert ret == 0, stdout
286+
287+
288+
# no error as inline suppressions are currently not being propagated back
289+
def test_duplicate(tmpdir):
290+
args = [
291+
'-q',
292+
'--template=simple',
293+
'--cppcheck-build-dir={}'.format(tmpdir),
294+
'--enable=all',
295+
'--inline-suppr',
296+
'proj-inline-suppress/duplicate.cpp'
297+
]
298+
299+
ret, stdout, stderr = cppcheck(args, cwd=__script_dir)
300+
lines = stderr.splitlines()
301+
assert lines == []
302+
assert stdout == ''
303+
assert ret == 0, stdout
304+
305+
306+
# no error as inline suppressions are currently not being propagated back
307+
def test_duplicate_cmd(tmpdir):
308+
args = [
309+
'-q',
310+
'--template=simple',
311+
'--cppcheck-build-dir={}'.format(tmpdir),
312+
'--enable=all',
313+
'--inline-suppr',
314+
'--suppress=unreadVariable',
315+
'proj-inline-suppress/4.c'
316+
]
317+
318+
ret, stdout, stderr = cppcheck(args, cwd=__script_dir)
319+
lines = stderr.splitlines()
320+
assert lines == []
321+
assert stdout == ''
322+
assert ret == 0, stdout
323+
324+
325+
# no error as inline suppressions are currently not being propagated back
326+
def test_duplicate_file(tmpdir):
327+
suppr_file = os.path.join(tmpdir, 'suppressions')
328+
with open(suppr_file, 'wt') as f:
329+
f.write('''unreadVariable''')
330+
331+
args = [
332+
'-q',
333+
'--template=simple',
334+
'--cppcheck-build-dir={}'.format(tmpdir),
335+
'--enable=all',
336+
'--inline-suppr',
337+
'--suppressions-list={}'.format(suppr_file),
338+
'proj-inline-suppress/4.c'
339+
]
340+
341+
ret, stdout, stderr = cppcheck(args, cwd=__script_dir)
342+
lines = stderr.splitlines()
343+
assert lines == []
344+
assert stdout == ''
345+
assert ret == 0, stdout

‎test/cli/other_test.py‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1346,4 +1346,48 @@ def test_rule(tmpdir):
13461346
lines = stderr.splitlines()
13471347
assert lines == [
13481348
"{}:4:0: style: found 'f' [rule]".format(test_file)
1349-
]
1349+
]
1350+
1351+
1352+
def test_duplicate_suppression(tmpdir):
1353+
test_file = os.path.join(tmpdir, 'file.cpp')
1354+
with open(test_file, 'wt'):
1355+
pass
1356+
1357+
exitcode, stdout, stderr = cppcheck(['-q', '--suppress=uninitvar', '--suppress=uninitvar', test_file])
1358+
assert exitcode == 1, stdout
1359+
assert stdout == "cppcheck: error: suppression 'uninitvar' already exists\n"
1360+
assert stderr == ''
1361+
1362+
1363+
def test_duplicate_suppressions_list(tmpdir):
1364+
suppr_file = os.path.join(tmpdir, 'suppressions')
1365+
with open(suppr_file, 'wt') as f:
1366+
f.write('''
1367+
uninitvar
1368+
uninitvar
1369+
''')
1370+
1371+
test_file = os.path.join(tmpdir, 'file.cpp')
1372+
with open(test_file, 'wt'):
1373+
pass
1374+
1375+
exitcode, stdout, stderr = cppcheck(['-q', '--suppressions-list={}'.format(suppr_file), test_file])
1376+
assert exitcode == 1, stdout
1377+
assert stdout == "cppcheck: error: suppression 'uninitvar' already exists\n"
1378+
assert stderr == ''
1379+
1380+
1381+
def test_duplicate_suppressions_mixed(tmpdir):
1382+
suppr_file = os.path.join(tmpdir, 'suppressions')
1383+
with open(suppr_file, 'wt') as f:
1384+
f.write('''uninitvar''')
1385+
1386+
test_file = os.path.join(tmpdir, 'file.cpp')
1387+
with open(test_file, 'wt'):
1388+
pass
1389+
1390+
exitcode, stdout, stderr = cppcheck(['-q', '--suppress=uninitvar', '--suppressions-list={}'.format(suppr_file), test_file])
1391+
assert exitcode == 1, stdout
1392+
assert stdout == "cppcheck: error: suppression 'uninitvar' already exists\n"
1393+
assert stderr == ''

‎test/cli/proj-inline-suppress/duplicate.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// cppcheck-suppress [unusedFunction,unusedFunction]
2+
void f()
3+
{
4+
}

‎test/testsuppressions.cpp‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,12 @@ class TestSuppressions : public TestFixture {
9797
TEST_CASE(suppressLocal);
9898

9999
TEST_CASE(suppressUnmatchedSuppressions);
100+
TEST_CASE(addSuppressionDuplicate);
101+
TEST_CASE(updateSuppressionState);
100102

101103
TEST_CASE(suppressionsParseXmlFile);
104+
105+
TEST_CASE(toString);
102106
}
103107

104108
void suppressionsBadId1() const {
@@ -1525,6 +1529,83 @@ class TestSuppressions : public TestFixture {
15251529
ASSERT_EQUALS("unknown element 'eid' in suppressions XML 'suppressparsexml.xml', expected id/fileName/lineNumber/symbolName/hash.", supprList.parseXmlFile(file.path().c_str()));
15261530
}
15271531
}
1532+
1533+
void addSuppressionDuplicate() {
1534+
SuppressionList supprs;
1535+
1536+
SuppressionList::Suppression s;
1537+
s.errorId = "uninitvar";
1538+
1539+
ASSERT_EQUALS("", supprs.addSuppression(s));
1540+
ASSERT_EQUALS("suppression 'uninitvar' already exists", supprs.addSuppression(s));
1541+
}
1542+
1543+
void updateSuppressionState() {
1544+
{
1545+
SuppressionList supprs;
1546+
1547+
SuppressionList::Suppression s;
1548+
s.errorId = "uninitVar";
1549+
ASSERT_EQUALS(false, supprs.updateSuppressionState(s));
1550+
}
1551+
{
1552+
SuppressionList supprs;
1553+
1554+
SuppressionList::Suppression s;
1555+
s.errorId = "uninitVar";
1556+
1557+
ASSERT_EQUALS("", supprs.addSuppression(s));
1558+
1559+
ASSERT_EQUALS(true, supprs.updateSuppressionState(s));
1560+
1561+
const std::list<SuppressionList::Suppression> l = supprs.getUnmatchedGlobalSuppressions(false);
1562+
ASSERT_EQUALS(1, l.size());
1563+
}
1564+
{
1565+
SuppressionList supprs;
1566+
1567+
SuppressionList::Suppression s;
1568+
s.errorId = "uninitVar";
1569+
s.matched = false;
1570+
1571+
ASSERT_EQUALS("", supprs.addSuppression(s));
1572+
1573+
s.matched = true;
1574+
ASSERT_EQUALS(true, supprs.updateSuppressionState(s));
1575+
1576+
const std::list<SuppressionList::Suppression> l = supprs.getUnmatchedGlobalSuppressions(false);
1577+
ASSERT_EQUALS(0, l.size());
1578+
}
1579+
}
1580+
1581+
void toString()
1582+
{
1583+
{
1584+
SuppressionList::Suppression s;
1585+
s.errorId = "unitvar";
1586+
ASSERT_EQUALS("unitvar", s.toString());
1587+
}
1588+
{
1589+
SuppressionList::Suppression s;
1590+
s.errorId = "unitvar";
1591+
s.fileName = "test.cpp";
1592+
ASSERT_EQUALS("unitvar:test.cpp", s.toString());
1593+
}
1594+
{
1595+
SuppressionList::Suppression s;
1596+
s.errorId = "unitvar";
1597+
s.fileName = "test.cpp";
1598+
s.lineNumber = 12;
1599+
ASSERT_EQUALS("unitvar:test.cpp:12", s.toString());
1600+
}
1601+
{
1602+
SuppressionList::Suppression s;
1603+
s.errorId = "unitvar";
1604+
s.symbolName = "sym";
1605+
ASSERT_EQUALS("unitvar:sym", s.toString());
1606+
}
1607+
1608+
}
15281609
};
15291610

15301611
REGISTER_TEST(TestSuppressions)

0 commit comments

Comments
 (0)