Fix defaulted and deleted functions#4540
Conversation
…ped everything after
Context:
```
struct S {
~S();
};
S::~S() = default;
void g() {
int j;
++j;
}
```
Everything after `S::~S() = default;` was skipped, so the uninitialized variables in g() weren't found.
Out-of-line destructors are useful e.g. when you have a forward declared unique_ptr in the .h,
and `= default` the destructor in the .cpp, so only the cpp needs to know the header for destructing
your unique_ptr (like in the pImpl-idiom)
Previous commit broke this test, but also provided the tools for a cleaner fix
`a::b f() = delete` triggered the final else in SymbolDatabase::addNewFunction, which sets tok to nullptr, effectively skipping to the end of the stream.
It was introduced in 0746c24 to fix a memory leak. But setting tok to nullptr, effectively skipping to the end, seems not needed. Previous commits fixes prevented some cases where you could enter the `else`. This commit is more of a fall back.
| } else { | ||
| scopeList.pop_back(); | ||
| *scope = nullptr; | ||
| *tok = nullptr; |
There was a problem hiding this comment.
This *tok = nullptr was the curlpit why the code after = delete wan't analysed at all. Which gave a false sense of security, because cppcheck complained about nothing, but also didn't notify me that something was broken.
I removed the nullptr to prevent it from being silent on future issues, but shouldn't it throw some InvalidAst or something? Similar for a few lines up, with the comment // syntax error?. There as well, tok ends up as the last token, effectively skipping the analysis.
Also, I'm not sure about the *scope = nullptr in this case. Why not keep the existing scope we already had?
There was a problem hiding this comment.
By the way: two unit tests entered this else: TestClass::testGetFileInfo and the 3rd case in TestValueFlow::valueFlowCrashIncompleteCode.
For this second case, what's the goal of the test? Just that it doesn't crash? Because the faulty behaviour was present here, but the unit test didn't show it...
There was a problem hiding this comment.
If we run into significant problems then yes some kind of internal error should be generated.
There was a problem hiding this comment.
Just that it doesn't crash?
Yes, its to make sure we dont crash on incomplete code.
`a::b f() = delete` triggered the final else in SymbolDatabase::addNewFunction, which sets tok to nullptr, effectively skipping to the end of the stream.
`a::b f() = delete` triggered the final else in SymbolDatabase::addNewFunction, which sets tok to nullptr, effectively skipping to the end of the stream.
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
it looks good to me do you think it's ready to merge?
The only doubt I have is whether an internal error is needed or not for the case that shouldn't be reached. I don't know, I'm new to this project and don't know to what extend this kind of unexpected behaviour should be reported back. I'd say it is at least better than before, and therefore ready to merge |
Can you please throw an internal error.. |
|
Thanks! Did you contribute to cppcheck before? If not.. I wonder what name you would like that I add in the AUTHORS file? |
|
No, first time contribution. You can add Gerbo Engels to the list. And thank you for maintaining this project! |
4 participants
All code after certain defaulted or deleted functions was skipped altogether, because in
SymbolDatabase::addNewFunctionit ended up setting*tok = nullptr, effectively skipping to the end.Case 1:
Case 2:
Case 1 is an out-of-line defaulted destructor, which should behave similar to an out-of-line constructor.
Case 2 needed a scoped return type to fail. First, during tokenization, treat a deleted function similar to regular functions (skipping the function call from the AST). Then during the symboldatabase phase, make sure the token and scope continue from where they left off.