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.

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

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

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

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

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

@firewave firewave changed the title TestSymbolDatabase: do not set Library directly in valueType1() TestSymbolDatabase: fixed wrong startPattern2 in valueType1() by not setting Library directly Jan 22, 2024
@firewave
Copy link
Collaborator Author

I have no idea how to fix this. So somebody else needs to take a look at this.

@firewave
Copy link
Collaborator Author

firewave commented Jan 22, 2024

I already verified in the debugger that the values in the Library object after reading the XML are matching the ones we set explicitly beforehand.

If I disable the first assert it will succeed on the second one and fail on the third.

@firewave firewave changed the title TestSymbolDatabase: fixed wrong startPattern2 in valueType1() by not setting Library directly Library: corrected usage of Container::startPattern2 in detectContainerInternal() Jan 22, 2024
@firewave firewave changed the title Library: corrected usage of Container::startPattern2 in detectContainerInternal() Library: corrected usage of Container::startPattern2 Jan 22, 2024
@firewave firewave changed the title Library: corrected usage of Container::startPattern2 Library: cleaned up usage of Container::startPattern2 Jan 22, 2024
@firewave firewave marked this pull request as ready for review January 22, 2024 19:05
@firewave firewave marked this pull request as draft January 22, 2024 20:52
@firewave firewave changed the title Library: cleaned up usage of Container::startPattern2 fixed #12378 FN containerOutOfBounds with scope operator / Library: cleaned up usage of Container::startPattern2 Jan 22, 2024
@firewave firewave marked this pull request as ready for review January 23, 2024 11:46
@chrchr-github chrchr-github merged commit a7086c5 into danmar:main Jan 23, 2024
64 checks passed
@firewave firewave deleted the symdb-con branch January 23, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants