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
Conversation
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. |
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
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.
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.
Use Iterate if the order does not matter.
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:
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:
Limitations
None.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.