Skip to content
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

Jump to bottom

fixed #12378 FN containerOutOfBounds with scope operator / Library: cleaned up usage of Container::startPattern2 #5908

Merged
merged 3 commits into from
Jan 23, 2024
+36 −29
Merged

fixed #12378 FN containerOutOfBounds with scope operator / Library: cleaned up usage of Container::startPattern2 #5908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
596e463
TestSymbolDatabase: avoid setting `Library` directly
firewave Jan 22, 2024
63dde63
Library: cleaned up usage of `Container::startPattern2`
firewave Jan 22, 2024
c08adf1
added test for #12378 FN containerOutOfBounds with scope operator
chrchr-github 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
4 changes: 3 additions & 1 deletion lib/library.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,9 @@ Library::Error Library::load(const tinyxml2::XMLDocument &doc)
const char* const startPattern = node->Attribute("startPattern");
if (startPattern) {
container.startPattern = startPattern;
container.startPattern2 = container.startPattern + " !!::";
container.startPattern2 = startPattern;
if (!endsWith(container.startPattern, '<'))
container.startPattern2 += " !!::";
}
const char* const endPattern = node->Attribute("endPattern");
if (endPattern)
Expand Down
7 changes: 7 additions & 0 deletions test/cfg/std.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4925,6 +4925,13 @@ std::string global_scope_std() // #12355
return ss.str();
}

::std::size_t global_scope_std2() // #12378
{
std::vector<::std::size_t> v;
// cppcheck-suppress containerOutOfBounds
return v.front();
}

void unique_lock_const_ref(std::mutex& m)
{
std::unique_lock lock(m);
Expand Down
54 changes: 26 additions & 28 deletions test/testsymboldatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8775,35 +8775,33 @@ class TestSymbolDatabase : public TestFixture {
}
{
// Container
Settings sC;
Library::Container c;
c.startPattern = "C";
c.startPattern2 = "C !!::";
sC.library.containers["C"] = c;
constexpr char xmldata[] = "<?xml version=\"1.0\"?>\n"
"<def>\n"
" <container id=\"C\" startPattern=\"C\"/>\n"
"</def>";
const Settings sC = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
ASSERT_EQUALS("container(C) *", typeOf("C*c=new C;","new","test.cpp",&sC));
ASSERT_EQUALS("container(C) *", typeOf("x=(C*)c;","(","test.cpp",&sC));
ASSERT_EQUALS("container(C)", typeOf("C c = C();","(","test.cpp",&sC));
}
{
// Container (vector)
Settings set;
Library::Container vector;
vector.startPattern = "Vector <";
vector.startPattern2 = "Vector !!::";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was wrong. The pattern is actually Vector < !!::.

The test now fails at the very first assert in this block so I think we might have a bug on our hands here as the pattern does not seem to make sense as code like Vector<:: should not appear.

Turns out this was a bug in introduced in an optimization in 16ebb90.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually - the code was wrong before. So it seems it wasn't my change after all.

The test also initially did not include a < - that was added by @danmar in f131a99.

The initial code to append this was also added by @danmar in 2a82968. The whole changes are unfortunately spread across several commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a SDB test, why not replace &set with &settings1, Vector with std::vector, test::string with std::string?

            ASSERT_EQUALS("signed int", typeOf("std::vector<int> v; v[0]=3;", "[", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string)", typeOf("{return std::string();}", "(", "test.cpp", &settings1));
            ASSERT_EQUALS(
                "container(std :: string|wstring|u16string|u32string)",
                typeOf("void foo(std::vector<std::string> v) { for (auto s: v) { x=s+s; } }", "s", "test.cpp", &settings1));
            ASSERT_EQUALS(
                "container(std :: string|wstring|u16string|u32string)",
                typeOf("void foo(std::vector<std::string> v) { for (auto s: v) { x=s+s; } }", "+", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string) &",
                          typeOf("std::vector<std::string> v; x = v.front();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string) *",
                          typeOf("std::vector<std::string> v; x = v.data();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("signed int &", typeOf("std::vector<int> v; x = v.front();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("signed int *", typeOf("std::vector<int> v; x = v.data();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("signed int * *", typeOf("std::vector<int*> v; x = v.data();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("iterator(std :: vector <)", typeOf("std::vector<int> v; x = v.begin();", "(", "test.cpp", &settings1));
            ASSERT_EQUALS("signed int &", typeOf("std::vector<int> v; x = *v.begin();", "*", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string)",
                          typeOf("void foo(){std::string s; return \"x\"+s;}", "+", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string)",
                          typeOf("void foo(){std::string s; return s+\"x\";}", "+", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string)",
                          typeOf("void foo(){std::string s; return 'x'+s;}", "+", "test.cpp", &settings1));
            ASSERT_EQUALS("container(std :: string|wstring|u16string|u32string)",
                          typeOf("void foo(){std::string s; return s+'x';}", "+", "test.cpp", &settings1));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might be a way. But I am not sure if that will fix the test (will try later).

It still seems there is something wrong here as the startPattern2 will never match in that case.

I assume if startPattern ends with < we should not generate startPattern2 at all. But the question is what we should do in that case. Just skip it or drop the trailing <, append !!:: and match it like the test previously did manually?

The original code before my optimization suggests that something different should have been matched since the Token::Match() was redundant which was most likely a copy&paste error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test strangely works when using std::vector which puzzles me. I want to know what the difference is.

Actually. The !!:: in startPattern2 would not make any sense since the last token would always match since it would never occur in real code. So in case it ends with a < we should not be appending ::!! at all.

Copy link
Collaborator Author

@firewave firewave Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I found the error in the changes. I forgot to terminate the encoded < character &lt - the ; is missing at the end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually. The !!:: in startPattern2 would not make any sense since the last token would always match since it would never occur in real code. So in case it ends with a < we should not be appending ::!! at all.

This will fix https://trac.cppcheck.net/ticket/12378
Feel free to take the test case from #5910

vector.type_templateArgNo = 0;
vector.arrayLike_indexOp = true;
vector.functions["front"] =
Library::Container::Function{Library::Container::Action::NO_ACTION, Library::Container::Yield::ITEM};
vector.functions["data"] =
Library::Container::Function{Library::Container::Action::NO_ACTION, Library::Container::Yield::BUFFER};
vector.functions["begin"] = Library::Container::Function{Library::Container::Action::NO_ACTION,
Library::Container::Yield::START_ITERATOR};
set.library.containers["Vector"] = vector;
Library::Container string;
string.startPattern = "test :: string";
string.startPattern2 = "test :: string !!::";
string.arrayLike_indexOp = string.stdStringLike = true;
set.library.containers["test::string"] = string;
constexpr char xmldata[] = "<?xml version=\"1.0\"?>\n"
"<def>\n"
" <container id=\"Vector\" startPattern=\"Vector &lt;\">\n"
" <type templateParameter=\"0\"/>\n"
" <access indexOperator=\"array-like\">\n"
" <function name=\"front\" yields=\"item\"/>\n"
" <function name=\"data\" yields=\"buffer\"/>\n"
" <function name=\"begin\" yields=\"start-iterator\"/>\n"
" </access>\n"
" </container>\n"
" <container id=\"test::string\" startPattern=\"test :: string\">\n"
" <type string=\"std-like\"/>\n"
" <access indexOperator=\"array-like\"/>\n"
" </container>\n"
"</def>";
const Settings set = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
ASSERT_EQUALS("signed int", typeOf("Vector<int> v; v[0]=3;", "[", "test.cpp", &set));
ASSERT_EQUALS("container(test :: string)", typeOf("{return test::string();}", "(", "test.cpp", &set));
ASSERT_EQUALS(
Expand Down Expand Up @@ -8876,11 +8874,11 @@ class TestSymbolDatabase : public TestFixture {
// return
{
// Container
Settings sC;
Library::Container c;
c.startPattern = "C";
c.startPattern2 = "C !!::";
sC.library.containers["C"] = c;
constexpr char xmldata[] = "<?xml version=\"1.0\"?>\n"
"<def>\n"
" <container id=\"C\" startPattern=\"C\"/>\n"
"</def>";
const Settings sC = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
ASSERT_EQUALS("container(C)", typeOf("C f(char *p) { char data[10]; return data; }", "return", "test.cpp", &sC));
}
// Smart pointer
Expand Down
Loading