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

Make Window destruction not rely on undefined behavior #9227

Merged
merged 5 commits into from May 12, 2021

Conversation

frosch123
Copy link
Member

Motivation / Problem

There are various loops in OpenTTD, which iterate over all windows, and conditionally close some.
Because closing windows propagates to child windows, iterating over all windows while closing them is not easy.

OpenTTD solves this problem by keeping windows in the window list, and just marking the entries as invalid.
Later the list is cleaned from invalid entries.

However, currently OpenTTD uses an intrusive double-linked list for windows. So the list pointers in the Window class must stay valid even after destruction of the window. This has some problems:

  • The memory for Windows is allocated with operator new, but deallocated with free().
  • Access to the z_front/back pointers in the destructor must be obfuscated, in the hope that the compiler optimisation does not break the list.

Description

This PR replaces the intrusive list with a non-intrusive std::list<Window*>.
Windows keep a reference to their list entry, and set the list entry to nullptr in the Window destructor, to mark the entry as invalid.

Other changes:

  • Because the iteration over lists is not quite the same in both directions, iterating backwards is slightly awkward. To use the more straight-forward forward-iteration when the iteration order does not matter, a third iteratable "Iterate" is added in addition to "IterateFromBack" and "IterateFromFront".

Limitations

None.

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')

@PeterN
Copy link
Member

PeterN commented May 9, 2021

The order for RelocateAllWindows() is important, but the current FromBack() is wrong. Probably out of scope for this change, but specifically the titlebar/toolbar should be handled before other normal windows so that the visible caption text is correct.

@TrueBrain
Copy link
Member

TrueBrain commented May 10, 2021

Code-wise it "looks fine" (I do not really understand the code, but I trust you on this :D). Testing shows it works, so YOLO.

My main issue is the commit messages :D But no other place to mention it .. so you get it in a comment :D

  • Cleanup: no need for all these labels. The window list supports deletion of arbitrary windows, while iterating over it.
    In general if you start adding more than one sentence in a commit message, it is time to hit the enter button :D This is the case here too. Suggestion:
Cleanup: remove unneeded labels in window functions.

The window list supports deletion of arbitrary windows, while iterating over it.
  • Codechange: remove excessive templating in favour of a const_cast, which will become unnecessary in following commits.
    Similar here, I would make the first line a bit more to the point and use the enter key:
Codechange: remove excessive templating in favour of a const_cast.

The const_cast be removed by a later commit anyway.

I rewrote the last part of the sentence, as initially it was unclear to me what would become unnecessary .. English can be a bit tricky to what "which" refers to in such sentences. But that might be a me-problem, ofc.

  • Codechange: when iterating over Windows starting at or behind a specific Window, use iterators instead of 'subranges'.
    This was a confusing commit message for me. Maybe something like:
    Codechange: use iterators instead of 'subranges' when iterating from a specific window
    Pretty sure things like "starting at or behind" was part of why I think it is confusing :D Also not sure that is all that relevant for the commit message :)

  • Codechange: be more explicit where the iteration order of windows matters.
    Initially I had a really hard time linking your commit message to the change. But in the end I realised what you are doing: set Iterate as default, and only use FromBack / FromFront when it explicit has to be either. This is not so much related to the commit message, but more to the order the diff is presented .. not something you can help. But a suggestion that might make it more obvious:

Codechange: only use IterateFromBack / IterateFromFront if order is important

Use Iterate when the order doesn't matter; it will pick what ever is easiest.

Or something.

The window list supports deletion of arbitrary windows, while iterating over it.
…ast.

The const_cast will be removed again in a later commit.
…a specific window.

Using iterators makes it easier to include or exclude the start window in the iteration.
@frosch123 frosch123 merged commit f6d5c01 into OpenTTD:master May 12, 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.

None yet

3 participants