Skip to content

testrunner: reduced usage of mutable Settings / some cleanups #5909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
firewave merged 2 commits into from
Jan 31, 2024
Merged

testrunner: reduced usage of mutable Settings / some cleanups #5909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3da9a25
testrunner: annotated `Settings` objects which should be `const`
firewave Jan 22, 2024
0564a11
testrunner: reduced usage of mutable `Settings`
firewave Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions test/fixture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,9 +461,11 @@ TestFixture::SettingsBuilder& TestFixture::SettingsBuilder::platform(Platform::T
TestFixture::SettingsBuilder& TestFixture::SettingsBuilder::libraryxml(const char xmldata[], std::size_t len)
{
tinyxml2::XMLDocument doc;
if (tinyxml2::XML_SUCCESS != doc.Parse(xmldata, len))
throw std::runtime_error("loading XML data failed");
if (settings.library.load(doc).errorcode != Library::ErrorCode::OK)
throw std::runtime_error("loading library XML failed");
const tinyxml2::XMLError xml_error = doc.Parse(xmldata, len);
if (tinyxml2::XML_SUCCESS != xml_error)
throw std::runtime_error(std::string("loading XML data failed - ") + tinyxml2::XMLDocument::ErrorIDToName(xml_error));
const Library::ErrorCode lib_error = settings.library.load(doc).errorcode;
if (lib_error != Library::ErrorCode::OK)
throw std::runtime_error("loading library XML failed - " + std::to_string(static_cast<int>(lib_error)));
return *this;
}
1 change: 0 additions & 1 deletion test/fixture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ class TestFixture : public ErrorLogger {
#define REGISTER_TEST( CLASSNAME ) namespace { CLASSNAME instance_ ## CLASSNAME; }

#define LOAD_LIB_2_EXE( LIB, NAME, EXE ) do { if (((LIB).load((EXE), NAME).errorcode != Library::ErrorCode::OK)) throw std::runtime_error("library '" + std::string(NAME) + "' not found"); } while (false)
#define LOAD_LIB_2( LIB, NAME ) LOAD_LIB_2_EXE(LIB, NAME, exename.c_str())

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

Expand Down
4 changes: 2 additions & 2 deletions test/testbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class TestBufferOverrun : public TestFixture {
TestBufferOverrun() : TestFixture("TestBufferOverrun") {}

private:
Settings settings0 = settingsBuilder().library("std.cfg").severity(Severity::warning).severity(Severity::style).severity(Severity::portability).build();
/*const*/ Settings settings0 = settingsBuilder().library("std.cfg").severity(Severity::warning).severity(Severity::style).severity(Severity::portability).build();

#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
void check_(const char* file, int line, const char code[], const char filename[] = "test.cpp") {
Expand Down Expand Up @@ -4191,7 +4191,7 @@ class TestBufferOverrun : public TestFixture {
" <arg nr=\"3\"/>\n"
" </function>\n"
"</def>";
Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).severity(Severity::warning).build();
/*const*/ Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).severity(Severity::warning).build();
settings.platform.sizeof_wchar_t = 4;

check("void f() {\n"
Expand Down
30 changes: 14 additions & 16 deletions test/testclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ class TestClass : public TestFixture {
TestClass() : TestFixture("TestClass") {}

private:
Settings settings0 = settingsBuilder().severity(Severity::style).library("std.cfg").build();
const Settings settings0 = settingsBuilder().severity(Severity::style).library("std.cfg").build();
const Settings settings1 = settingsBuilder().severity(Severity::warning).library("std.cfg").build();
const Settings settings2 = settingsBuilder().severity(Severity::style).library("std.cfg").certainty(Certainty::inconclusive).build();
const Settings settings3 = settingsBuilder().severity(Severity::style).library("std.cfg").severity(Severity::warning).build();

void run() override {
TEST_CASE(virtualDestructor1); // Base class not found => no error
Expand Down Expand Up @@ -731,15 +733,15 @@ class TestClass : public TestFixture {
// Clear the error log
errout.str("");

Preprocessor preprocessor(settings0);
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was relying on the modifications checkVirtualDestructor() did.

Preprocessor preprocessor(settings3);

// Tokenize..
Tokenizer tokenizer(settings0, this, &preprocessor);
Tokenizer tokenizer(settings3, this, &preprocessor);
std::istringstream istr(code);
ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);

// Check..
CheckClass checkClass(&tokenizer, &settings0, this);
CheckClass checkClass(&tokenizer, &settings3, this);
checkClass.copyconstructors();
}

Expand Down Expand Up @@ -2617,19 +2619,17 @@ class TestClass : public TestFixture {
// Clear the error log
errout.str("");

// TODO: subsequent tests depend on these changes - should use SettingsBuilder
settings0.certainty.setEnabled(Certainty::inconclusive, inconclusive);
settings0.severity.enable(Severity::warning);
const Settings s = settingsBuilder(settings0).certainty(Certainty::inconclusive, inconclusive).severity(Severity::warning).build();

Preprocessor preprocessor(settings0);
Preprocessor preprocessor(s);

// Tokenize..
Tokenizer tokenizer(settings0, this, &preprocessor);
Tokenizer tokenizer(s, this, &preprocessor);
std::istringstream istr(code);
ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);

// Check..
CheckClass checkClass(&tokenizer, &settings0, this);
CheckClass checkClass(&tokenizer, &s, this);
checkClass.virtualDestructor();
}

Expand Down Expand Up @@ -7554,16 +7554,14 @@ class TestClass : public TestFixture {
errout.str("");

// Check..
settings0.certainty.setEnabled(Certainty::inconclusive, true);

Preprocessor preprocessor(settings0);
Preprocessor preprocessor(settings2);

// Tokenize..
Tokenizer tokenizer(settings0, this, &preprocessor);
Tokenizer tokenizer(settings2, this, &preprocessor);
std::istringstream istr(code);
ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);

CheckClass checkClass(&tokenizer, &settings0, this);
CheckClass checkClass(&tokenizer, &settings2, this);
checkClass.initializerListOrder();
}

Expand Down Expand Up @@ -8738,7 +8736,7 @@ class TestClass : public TestFixture {
// Clear the error log
errout.str("");

Settings settings = settingsBuilder().severity(Severity::warning).build();
/*const*/ Settings settings = settingsBuilder().severity(Severity::warning).build();
settings.safeChecks.classes = true;

Preprocessor preprocessor(settings);
Expand Down
2 changes: 1 addition & 1 deletion test/testcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class TestCondition : public TestFixture {

private:
const Settings settings0 = settingsBuilder().library("qt.cfg").library("std.cfg").severity(Severity::style).severity(Severity::warning).build();
Settings settings1 = settingsBuilder().severity(Severity::style).severity(Severity::warning).build();
/*const*/ Settings settings1 = settingsBuilder().severity(Severity::style).severity(Severity::warning).build();

void run() override {
const char cfg[] = "<?xml version=\"1.0\"?>\n"
Expand Down
2 changes: 1 addition & 1 deletion test/testexceptionsafety.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class TestExceptionSafety : public TestFixture {
TestExceptionSafety() : TestFixture("TestExceptionSafety") {}

private:
Settings settings;
/*const*/ Settings settings;

void run() override {
settings.severity.fill();
Expand Down
2 changes: 1 addition & 1 deletion test/testfunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1837,7 +1837,7 @@ class TestFunctions : public TestFixture {
}

void checkLibraryMatchFunctions() {
Settings s = settingsBuilder(settings).checkLibrary().debugwarnings().build();
/*const*/ Settings s = settingsBuilder(settings).checkLibrary().debugwarnings().build();
s.daca = true;

check("void f() {\n"
Expand Down
2 changes: 1 addition & 1 deletion test/testgarbage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class TestGarbage : public TestFixture {
TestGarbage() : TestFixture("TestGarbage") {}

private:
Settings settings = settingsBuilder().debugwarnings().build();
/*const*/ Settings settings = settingsBuilder().debugwarnings().build();

void run() override {
settings.severity.fill();
Expand Down
2 changes: 1 addition & 1 deletion test/testinternal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class TestInternal : public TestFixture {
TestInternal() : TestFixture("TestInternal") {}

private:
Settings settings;
/*const*/ Settings settings;

void run() override {
settings.addEnabled("internal");
Expand Down
2 changes: 1 addition & 1 deletion test/testio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class TestIO : public TestFixture {

private:
const Settings settings = settingsBuilder().library("std.cfg").library("windows.cfg").library("qt.cfg").build();
Settings settings1 = settingsBuilder().library("std.cfg").library("windows.cfg").library("qt.cfg").build();
/*const*/ Settings settings1 = settingsBuilder().library("std.cfg").library("windows.cfg").library("qt.cfg").build();

void run() override {
TEST_CASE(coutCerrMisusage);
Expand Down
33 changes: 13 additions & 20 deletions test/testother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,9 @@ class TestOther : public TestFixture {
TestOther() : TestFixture("TestOther") {}

private:
Settings _settings;
/*const*/ Settings _settings = settingsBuilder().library("std.cfg").build();

void run() override {
LOAD_LIB_2(_settings.library, "std.cfg");


TEST_CASE(emptyBrackets);

TEST_CASE(zeroDiv1);
Expand Down Expand Up @@ -355,8 +352,7 @@ class TestOther : public TestFixture {
}

void checkInterlockedDecrement(const char code[]) {
Settings settings;
settings.platform.type = Platform::Type::Win32A;
/*const*/ Settings settings = settingsBuilder().platform(Platform::Type::Win32A).build();

check(code, nullptr, false, true, false, &settings);
}
Expand Down Expand Up @@ -1905,7 +1901,7 @@ class TestOther : public TestFixture {
// Clear the error buffer..
errout.str("");

Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::portability, portability).certainty(Certainty::inconclusive, inconclusive).build();
/*const*/ Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::portability, portability).certainty(Certainty::inconclusive, inconclusive).build();
settings.platform.defaultSign = 's';

Preprocessor preprocessor(settings);
Expand Down Expand Up @@ -2204,7 +2200,7 @@ class TestOther : public TestFixture {
"}\n");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 't' should be passed by const reference.\n", errout.str());

Settings settings0 = settingsBuilder(_settings).platform(Platform::Type::Unix64).build();
/*const*/ Settings settings0 = settingsBuilder(_settings).platform(Platform::Type::Unix64).build();
check("struct S {\n" // #12138
" union {\n"
" int a = 0;\n"
Expand Down Expand Up @@ -2243,7 +2239,7 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 's' should be passed by const reference.\n",
errout.str());

Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build();
/*const*/ Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build();
check("using ui64 = unsigned __int64;\n"
"ui64 Test(ui64 one, ui64 two) { return one + two; }\n",
/*filename*/ nullptr, /*inconclusive*/ true, /*runSimpleChecks*/ true, /*verbose*/ false, &settings1);
Expand Down Expand Up @@ -2390,11 +2386,11 @@ class TestOther : public TestFixture {
"};\n"
"void f(X x) {}";

Settings s32 = settingsBuilder(_settings).platform(Platform::Type::Unix32).build();
/*const*/ Settings s32 = settingsBuilder(_settings).platform(Platform::Type::Unix32).build();
check(code, &s32);
ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by const reference.\n", errout.str());

Settings s64 = settingsBuilder(_settings).platform(Platform::Type::Unix64).build();
/*const*/ Settings s64 = settingsBuilder(_settings).platform(Platform::Type::Unix64).build();
check(code, &s64);
ASSERT_EQUALS("", errout.str());
}
Expand Down Expand Up @@ -4929,7 +4925,7 @@ class TestOther : public TestFixture {
" <arg nr=\"1\"/>\n"
" </function>\n"
"</def>";
Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
/*const*/ Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();

check("void foo() {\n"
" exit(0);\n"
Expand Down Expand Up @@ -6619,7 +6615,7 @@ class TestOther : public TestFixture {
" <arg nr=\"2\"/>\n"
" </function>\n"
"</def>";
Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
/*const*/ Settings settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();

check("void foo() {\n"
" if (x() || x()) {}\n"
Expand Down Expand Up @@ -7260,7 +7256,7 @@ class TestOther : public TestFixture {
const char code[] = "void foo(bool flag) {\n"
" bar( (flag) ? ~0u : ~0ul);\n"
"}";
Settings settings = _settings;
/*const*/ Settings settings = _settings;
settings.platform.sizeof_int = 4;
settings.platform.int_bit = 32;

Expand Down Expand Up @@ -8199,10 +8195,9 @@ class TestOther : public TestFixture {
}

{
Settings s = settingsBuilder().checkUnusedTemplates().build();
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary as checkUnusedTemplates is true by default.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now uses _settings which has std.cfg loaded but I do think this is not an issue.

check("template<int n> void foo(unsigned int x) {\n"
"if (x <= 0);\n"
"}", &s);
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
}

Expand All @@ -8217,7 +8212,7 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'value' is less than zero.\n", errout.str());

// #9040
Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build();
/*const*/ Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build();
check("using BOOL = unsigned;\n"
"int i;\n"
"bool f() {\n"
Expand Down Expand Up @@ -11093,13 +11088,11 @@ class TestOther : public TestFixture {
}

void forwardAndUsed() {
Settings s = settingsBuilder().checkUnusedTemplates().build();
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary as checkUnusedTemplates is true by default.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now uses _settings which has std.cfg loaded but I do think this is not an issue.


check("template<typename T>\n"
"void f(T && t) {\n"
" g(std::forward<T>(t));\n"
" T s = t;\n"
"}", &s);
"}");
ASSERT_EQUALS("[test.cpp:4]: (warning) Access of forwarded variable 't'.\n", errout.str());
}

Expand Down
Loading