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

Codechange: document/fix virtual function resolving in con/destructors #9257

Merged
merged 2 commits into from May 13, 2021

Conversation

rubidium42
Copy link
Contributor

Motivation / Problem

Virtual functions should not be invoked from a constructor or destructor of the same class. Confusingly, virtual functions are resolved statically (not dynamically) in constructors and destructors for the same class. The call should be made explicitly static by qualifying it using the scope resolution operator. https://lgtm.com/rules/2159620519/

Description

Added documentation and the explicit static call where applicable and removed some pointless code. See the commit messages for more details.

Limitations

The problems with the TCPConnecters are not solved as they are simply broken for failure cases when having no threads. #9255 has been made to solve that.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

…are resolved statically

This as during construction the sub class has not been initialized yet, and
during destruction the sub class has already been destroyed, so the overriding
virtual function would be accessing uninitialized data.
…tions statically in destructors

In the destructors of many of the network related classes Close() is called, just like the
top class in that hierarchy. However, due to virtual functions getting resolved statically
in the destructor it would always call the empty Close() of the top class.
Document the other cases where a virtual call is resolved statically.
@rubidium42 rubidium42 merged commit 187a3f2 into OpenTTD:master May 13, 2021
@rubidium42 rubidium42 deleted the virtual-in-cdtor branch May 13, 2021 08:03
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.

None yet

2 participants