Skip to content

fix false positive of virtual function call - #3553

Merged
danmar merged 2 commits into
cppcheck-opensource:mainfrom
nomick:virtual-call-fix
Nov 11, 2021
Merged

fix false positive of virtual function call#3553
danmar merged 2 commits into
cppcheck-opensource:mainfrom
nomick:virtual-call-fix

Conversation

@nomick

@nomick nomick commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

Cppcheck issues a warning if a virtual call is called from a constructor or destructor. If the method is just overridden and marked final, this is a false positive.

@chrchr-github

Copy link
Copy Markdown
Collaborator

Do you have a test case for your fix?

@nomick

nomick commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author

The following should be ok (giving a virtualCallInConstructor)

class T {
public:
    virtual void vOk() {}
};

class T2 : public T {
public:
    T2() { vOk(); }
    void vOk() override final {}
};

I am just not sure where to add a unit test.

@danmar

danmar commented Nov 8, 2021

Copy link
Copy Markdown
Collaborator

You can add a unit test in testclass.cpp. If you search for "Dynamic binding is not used" in that file you can find some tests in virtualFunctionCallInConstructor. I suggest you add this test at the end of that function.

@nomick

nomick commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author

That was easy ;) Test verified to fail on current master. Works with patch...

@danmar danmar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm. if CI succeeds I think this can be merged.

@firewave

firewave commented Nov 8, 2021

Copy link
Copy Markdown
Collaborator

https://trac.cppcheck.net/ticket/9333 - how did this regress?

@danmar

danmar commented Nov 11, 2021

Copy link
Copy Markdown
Collaborator

https://trac.cppcheck.net/ticket/9333 - how did this regress?

hmm not sure I think my comment in that ticket is valid. It should probably rely on CTU.

@danmar
danmar merged commit 5865b05 into cppcheck-opensource:main Nov 11, 2021
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.

4 participants