Skip to content

Commit d89e241

Browse files
firewavefirewave
authored
mitigated most bugprone-unused-return-value clang-tidy warnings in test code when we enable it for *all* functions (#6673)
We should not have any unchecked returns within the test code to make sure everything is working as expected (there might be a case that should also apply to the production code). Unfortunately enabling all functions is abusing this check and it will produce a lot of "false positives" (e.g. `erase()`/`insert()` of containers or usage of stream insertion operators) so this cannot be enabled by default. Since `llvm:Regex` does not support negative lookahead we cannot configure the check to exclude functions.
1 parent cd20dce commit d89e241

27 files changed

Lines changed: 565 additions & 553 deletions

‎test/.clang-tidy‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
InheritParentConfig: true
3+
CheckOptions:
4+
#- key: bugprone-unused-return-value.CheckedFunctions
5+
# value: '::.*'
6+
- key: bugprone-unused-return-value.AllowCastToVoid
7+
value: 'true'

‎test/fixture.cpp‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,9 @@ std::string TestFixture::deleteLineNumber(const std::string &message)
216216
return result;
217217
}
218218

219-
void TestFixture::assertEqualsWithoutLineNumbers(const char * const filename, const unsigned int linenr, const std::string &expected, const std::string &actual, const std::string &msg) const
219+
bool TestFixture::assertEqualsWithoutLineNumbers(const char * const filename, const unsigned int linenr, const std::string &expected, const std::string &actual, const std::string &msg) const
220220
{
221-
assertEquals(filename, linenr, deleteLineNumber(expected), deleteLineNumber(actual), msg);
221+
return assertEquals(filename, linenr, deleteLineNumber(expected), deleteLineNumber(actual), msg);
222222
}
223223

224224
bool TestFixture::assertEquals(const char * const filename, const unsigned int linenr, const char expected[], const std::string& actual, const std::string &msg) const
@@ -237,20 +237,21 @@ bool TestFixture::assertEquals(const char * const filename, const unsigned int l
237237
bool TestFixture::assertEquals(const char * const filename, const unsigned int linenr, const long long expected, const long long actual, const std::string &msg) const
238238
{
239239
if (expected != actual) {
240-
assertEquals(filename, linenr, std::to_string(expected), std::to_string(actual), msg);
240+
return assertEquals(filename, linenr, std::to_string(expected), std::to_string(actual), msg);
241241
}
242-
return expected == actual;
242+
return true;
243243
}
244244

245-
void TestFixture::assertEqualsDouble(const char * const filename, const unsigned int linenr, const double expected, const double actual, const double tolerance, const std::string &msg) const
245+
bool TestFixture::assertEqualsDouble(const char * const filename, const unsigned int linenr, const double expected, const double actual, const double tolerance, const std::string &msg) const
246246
{
247247
if (expected < (actual - tolerance) || expected > (actual + tolerance)) {
248248
std::ostringstream ostr1;
249249
ostr1 << expected;
250250
std::ostringstream ostr2;
251251
ostr2 << actual;
252-
assertEquals(filename, linenr, ostr1.str(), ostr2.str(), msg);
252+
return assertEquals(filename, linenr, ostr1.str(), ostr2.str(), msg);
253253
}
254+
return false;
254255
}
255256

256257
void TestFixture::todoAssertEquals(const char * const filename, const unsigned int linenr,
@@ -264,7 +265,7 @@ void TestFixture::todoAssertEquals(const char * const filename, const unsigned i
264265

265266
++succeeded_todos_counter;
266267
} else {
267-
assertEquals(filename, linenr, current, actual);
268+
(void)assertEquals(filename, linenr, current, actual);
268269
++todos_counter;
269270
}
270271
}

‎test/fixture.h‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,12 @@ class TestFixture : public ErrorLogger {
9595
void assertEqualsFailed(const char* filename, unsigned int linenr, const std::string& expected, const std::string& actual, const std::string& msg) const;
9696

9797
bool assertEquals(const char * filename, unsigned int linenr, const std::string &expected, const std::string &actual, const std::string &msg = emptyString) const;
98-
void assertEqualsWithoutLineNumbers(const char * filename, unsigned int linenr, const std::string &expected, const std::string &actual, const std::string &msg = emptyString) const;
98+
bool assertEqualsWithoutLineNumbers(const char * filename, unsigned int linenr, const std::string &expected, const std::string &actual, const std::string &msg = emptyString) const;
9999
bool assertEquals(const char * filename, unsigned int linenr, const char expected[], const std::string& actual, const std::string &msg = emptyString) const;
100100
bool assertEquals(const char * filename, unsigned int linenr, const char expected[], const char actual[], const std::string &msg = emptyString) const;
101101
bool assertEquals(const char * filename, unsigned int linenr, const std::string& expected, const char actual[], const std::string &msg = emptyString) const;
102102
bool assertEquals(const char * filename, unsigned int linenr, long long expected, long long actual, const std::string &msg = emptyString) const;
103-
void assertEqualsDouble(const char * filename, unsigned int linenr, double expected, double actual, double tolerance, const std::string &msg = emptyString) const;
103+
bool assertEqualsDouble(const char * filename, unsigned int linenr, double expected, double actual, double tolerance, const std::string &msg = emptyString) const;
104104

105105
void todoAssertEquals(const char * filename, unsigned int linenr, const std::string &wanted,
106106
const std::string &current, const std::string &actual) const;
@@ -296,30 +296,30 @@ class TestInstance {
296296
};
297297

298298
// TODO: most asserts do not actually assert i.e. do not return
299-
#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } } while (false)
300-
#define ASSERT( CONDITION ) if (!assert_(__FILE__, __LINE__, (CONDITION))) return
301-
#define ASSERT_LOC( CONDITION, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION))
302-
#define CHECK_EQUALS( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, (EXPECTED), (ACTUAL))
299+
#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } } while (false)
300+
#define ASSERT( CONDITION ) if (!assert_(__FILE__, __LINE__, (CONDITION))) return
301+
#define ASSERT_LOC( CONDITION, FILE_, LINE_ ) (void)assert_(FILE_, LINE_, (CONDITION))
302+
#define CHECK_EQUALS( EXPECTED, ACTUAL ) (void)assertEquals(__FILE__, __LINE__, (EXPECTED), (ACTUAL))
303303
// *INDENT-OFF*
304304
#define ASSERT_EQUALS( EXPECTED, ACTUAL ) do { try { if (!assertEquals(__FILE__, __LINE__, (EXPECTED), (ACTUAL))) return; } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } while (false)
305305
// *INDENT-ON*
306-
#define ASSERT_EQUALS_WITHOUT_LINENUMBERS( EXPECTED, ACTUAL ) assertEqualsWithoutLineNumbers(__FILE__, __LINE__, EXPECTED, ACTUAL)
307-
#define ASSERT_EQUALS_DOUBLE( EXPECTED, ACTUAL, TOLERANCE ) assertEqualsDouble(__FILE__, __LINE__, EXPECTED, ACTUAL, TOLERANCE)
308-
#define ASSERT_EQUALS_LOC_MSG( EXPECTED, ACTUAL, MSG, FILE_, LINE_ ) assertEquals(FILE_, LINE_, EXPECTED, ACTUAL, MSG)
309-
#define ASSERT_EQUALS_MSG( EXPECTED, ACTUAL, MSG ) assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL, MSG)
310-
#define ASSERT_EQUALS_ENUM( EXPECTED, ACTUAL ) if (!assertEqualsEnum(__FILE__, __LINE__, (EXPECTED), (ACTUAL))) return
311-
#define ASSERT_THROW( CMD, EXCEPTION ) do { try { CMD; assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&) {} catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
312-
#define ASSERT_THROW_EQUALS( CMD, EXCEPTION, EXPECTED ) do { try { CMD; assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&e) { assertEquals(__FILE__, __LINE__, EXPECTED, e.errorMessage); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
313-
#define ASSERT_THROW_EQUALS_2( CMD, EXCEPTION, EXPECTED ) do { try { CMD; assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&e) { assertEquals(__FILE__, __LINE__, EXPECTED, e.what()); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
314-
#define ASSERT_THROW_INTERNAL( CMD, TYPE ) do { try { CMD; assertThrowFail(__FILE__, __LINE__); } catch (const InternalError& e) { assertEqualsEnum(__FILE__, __LINE__, InternalError::TYPE, e.type); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
315-
#define ASSERT_THROW_INTERNAL_EQUALS( CMD, TYPE, EXPECTED ) do { try { CMD; assertThrowFail(__FILE__, __LINE__); } catch (const InternalError& e) { assertEqualsEnum(__FILE__, __LINE__, InternalError::TYPE, e.type); assertEquals(__FILE__, __LINE__, EXPECTED, e.errorMessage); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
316-
#define ASSERT_NO_THROW( CMD ) do { try { CMD; } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } while (false)
317-
#define TODO_ASSERT_THROW( CMD, EXCEPTION ) do { try { CMD; } catch (const EXCEPTION&) {} catch (...) { assertThrow(__FILE__, __LINE__); } } while (false)
306+
#define ASSERT_EQUALS_WITHOUT_LINENUMBERS( EXPECTED, ACTUAL ) (void)assertEqualsWithoutLineNumbers(__FILE__, __LINE__, EXPECTED, ACTUAL)
307+
#define ASSERT_EQUALS_DOUBLE( EXPECTED, ACTUAL, TOLERANCE ) (void)assertEqualsDouble(__FILE__, __LINE__, EXPECTED, ACTUAL, TOLERANCE)
308+
#define ASSERT_EQUALS_LOC_MSG( EXPECTED, ACTUAL, MSG, FILE_, LINE_ ) (void)assertEquals(FILE_, LINE_, EXPECTED, ACTUAL, MSG)
309+
#define ASSERT_EQUALS_MSG( EXPECTED, ACTUAL, MSG ) (void)assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL, MSG)
310+
#define ASSERT_EQUALS_ENUM( EXPECTED, ACTUAL ) if (!assertEqualsEnum(__FILE__, __LINE__, (EXPECTED), (ACTUAL))) return
311+
#define ASSERT_THROW( CMD, EXCEPTION ) do { try { (void)(CMD); assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&) {} catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
312+
#define ASSERT_THROW_EQUALS( CMD, EXCEPTION, EXPECTED ) do { try { (void)(CMD); assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&e) { (void)assertEquals(__FILE__, __LINE__, EXPECTED, e.errorMessage); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
313+
#define ASSERT_THROW_EQUALS_2( CMD, EXCEPTION, EXPECTED ) do { try { (void)(CMD); assertThrowFail(__FILE__, __LINE__); } catch (const EXCEPTION&e) { (void)assertEquals(__FILE__, __LINE__, EXPECTED, e.what()); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
314+
#define ASSERT_THROW_INTERNAL( CMD, TYPE ) do { try { (void)(CMD); assertThrowFail(__FILE__, __LINE__); } catch (const InternalError& e) { (void)assertEqualsEnum(__FILE__, __LINE__, InternalError::TYPE, e.type); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
315+
#define ASSERT_THROW_INTERNAL_EQUALS( CMD, TYPE, EXPECTED ) do { try { (void)(CMD); assertThrowFail(__FILE__, __LINE__); } catch (const InternalError& e) { (void)assertEqualsEnum(__FILE__, __LINE__, InternalError::TYPE, e.type); (void)assertEquals(__FILE__, __LINE__, EXPECTED, e.errorMessage); } catch (...) { assertThrowFail(__FILE__, __LINE__); } } while (false)
316+
#define ASSERT_NO_THROW( CMD ) do { try { (void)(CMD); } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } while (false)
317+
#define TODO_ASSERT_THROW( CMD, EXCEPTION ) do { try { (void)(CMD); } catch (const EXCEPTION&) {} catch (...) { assertThrow(__FILE__, __LINE__); } } while (false)
318318
#define TODO_ASSERT( CONDITION ) do { const bool condition=(CONDITION); todoAssertEquals(__FILE__, __LINE__, true, false, condition); } while (false)
319319
#define TODO_ASSERT_EQUALS( WANTED, CURRENT, ACTUAL ) todoAssertEquals(__FILE__, __LINE__, WANTED, CURRENT, ACTUAL)
320-
#define EXPECT_EQ( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL)
320+
#define EXPECT_EQ( EXPECTED, ACTUAL ) (void)assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL)
321321
#define REGISTER_TEST( CLASSNAME ) namespace { class CLASSNAME ## Instance : public TestInstance { public: CLASSNAME ## Instance() : TestInstance(#CLASSNAME) {} TestFixture* create() override { impl.reset(new CLASSNAME); return impl.get(); } }; CLASSNAME ## Instance instance_ ## CLASSNAME; }
322322

323-
#define PLATFORM( P, T ) do { std::string errstr; assertEquals(__FILE__, __LINE__, true, P.set(Platform::toString(T), errstr, {exename}), errstr); } while (false)
323+
#define PLATFORM( P, T ) do { std::string errstr; (void)assertEquals(__FILE__, __LINE__, true, P.set(Platform::toString(T), errstr, {exename}), errstr); } while (false)
324324

325325
#endif // fixtureH

‎test/testastutils.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -231,10 +231,10 @@ class TestAstUtils : public TestFixture {
231231

232232
void isVariableChangedTest() {
233233
// #8211 - no lhs for >> , do not crash
234-
isVariableChanged("void f() {\n"
235-
" int b;\n"
236-
" if (b) { (int)((INTOF(8))result >> b); }\n"
237-
"}", "if", "}");
234+
(void)isVariableChanged("void f() {\n"
235+
" int b;\n"
236+
" if (b) { (int)((INTOF(8))result >> b); }\n"
237+
"}", "if", "}");
238238
// #9235
239239
ASSERT_EQUALS(false,
240240
isVariableChanged("void f() {\n"

‎test/testbufferoverrun.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5204,7 +5204,7 @@ class TestBufferOverrun : public TestFixture {
52045204
std::list<Check::FileInfo*> fileInfo;
52055205
Check& c = getCheck<CheckBufferOverrun>();
52065206
fileInfo.push_back(c.getFileInfo(tokenizer, settings0));
5207-
c.analyseWholeProgram(ctu, fileInfo, settings0, *this);
5207+
c.analyseWholeProgram(ctu, fileInfo, settings0, *this); // TODO: check result
52085208
while (!fileInfo.empty()) {
52095209
delete fileInfo.back();
52105210
fileInfo.pop_back();

‎test/testclangimport.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1363,7 +1363,7 @@ class TestClangImport : public TestFixture {
13631363
"`-CXXConstructorDecl 0x5603791b5600 parent 0x560379197110 prev 0x5603791974b8 <line:3:1, col:55> col:47 b 'void (A<type-parameter-0-0, type-parameter-0-1> &)'\n"
13641364
" |-ParmVarDecl 0x5603791b5570 <col:49, col:51> col:52 'A<type-parameter-0-0, type-parameter-0-1> &'\n"
13651365
" `-CompoundStmt 0x5603791b5700 <col:54, col:55>\n";
1366-
parse(clang); // don't crash
1366+
(void)parse(clang); // don't crash
13671367
}
13681368
};
13691369

‎test/testclass.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8933,7 +8933,7 @@ class TestClass : public TestFixture {
89338933
}
89348934

89358935
// Check code..
8936-
check.analyseWholeProgram(nullptr, fileInfo, settingsDefault, *this);
8936+
check.analyseWholeProgram(nullptr, fileInfo, settingsDefault, *this); // TODO: check result
89378937

89388938
while (!fileInfo.empty()) {
89398939
delete fileInfo.back();

0 commit comments

Comments
 (0)