From f6d5c0136e5ac130fd887daf1b07c13a160c7fc3 Mon Sep 17 00:00:00 2001 From: frosch Date: Sun, 9 May 2021 17:12:34 +0200 Subject: Codechange: make Window destruction not rely on undefined behavior. --- src/window.cpp | 136 ++++++++++--------------------------------------------- src/window_gui.h | 67 +++++++++++++++------------ 2 files changed, 63 insertions(+), 140 deletions(-) diff --git a/src/window.cpp b/src/window.cpp index 54671f90b..af4721445 100644 --- a/src/window.cpp +++ b/src/window.cpp @@ -53,10 +53,8 @@ static Point _drag_delta; ///< delta between mouse cursor and upper left corner static Window *_mouseover_last_w = nullptr; ///< Window of the last OnMouseOver event. static Window *_last_scroll_window = nullptr; ///< Window of the last scroll event. -/** List of windows opened at the screen sorted from the front. */ -Window *_z_front_window = nullptr; -/** List of windows opened at the screen sorted from the back. */ -Window *_z_back_window = nullptr; +/** List of windows opened at the screen sorted from the front to back. */ +WindowList _z_windows; /** If false, highlight is white, otherwise the by the widget defined colour. */ bool _window_highlight_colour = false; @@ -1109,16 +1107,7 @@ Window::~Window() free(this->nested_array); // Contents is released through deletion of #nested_root. delete this->nested_root; - /* - * Make fairly sure that this is written, and not "optimized" away. - * The delete operator is overwritten to not delete it; the deletion - * happens at a later moment in time after the window has been - * removed from the list of windows to prevent issues with items - * being removed during the iteration as not one but more windows - * may be removed by a single call to ~Window by means of the - * DeleteChildWindows function. - */ - const_cast(this->window_class) = WC_INVALID; + *this->z_position = nullptr; } /** @@ -1231,7 +1220,7 @@ void ChangeWindowOwner(Owner old_owner, Owner new_owner) } } -static void BringWindowToFront(Window *w); +static void BringWindowToFront(Window *w, bool dirty = true); /** * Find a window and make it the relative top-window on the screen. @@ -1351,90 +1340,23 @@ static uint GetWindowZPriority(WindowClass wc) } } -/** - * Adds a window to the z-ordering, according to its z-priority. - * @param w Window to add - */ -static void AddWindowToZOrdering(Window *w) -{ - assert(w->z_front == nullptr && w->z_back == nullptr); - - if (_z_front_window == nullptr) { - /* It's the only window. */ - _z_front_window = _z_back_window = w; - w->z_front = w->z_back = nullptr; - } else { - /* Search down the z-ordering for its location. */ - Window *v = _z_front_window; - uint last_z_priority = UINT_MAX; - (void)last_z_priority; // Unused without asserts - while (v != nullptr && (v->window_class == WC_INVALID || GetWindowZPriority(v->window_class) > GetWindowZPriority(w->window_class))) { - if (v->window_class != WC_INVALID) { - /* Sanity check z-ordering, while we're at it. */ - assert(last_z_priority >= GetWindowZPriority(v->window_class)); - last_z_priority = GetWindowZPriority(v->window_class); - } - - v = v->z_back; - } - - if (v == nullptr) { - /* It's the new back window. */ - w->z_front = _z_back_window; - w->z_back = nullptr; - _z_back_window->z_back = w; - _z_back_window = w; - } else if (v == _z_front_window) { - /* It's the new front window. */ - w->z_front = nullptr; - w->z_back = _z_front_window; - _z_front_window->z_front = w; - _z_front_window = w; - } else { - /* It's somewhere else in the z-ordering. */ - w->z_front = v->z_front; - w->z_back = v; - v->z_front->z_back = w; - v->z_front = w; - } - } -} - - -/** - * Removes a window from the z-ordering. - * @param w Window to remove - */ -static void RemoveWindowFromZOrdering(Window *w) -{ - if (w->z_front == nullptr) { - assert(_z_front_window == w); - _z_front_window = w->z_back; - } else { - w->z_front->z_back = w->z_back; - } - - if (w->z_back == nullptr) { - assert(_z_back_window == w); - _z_back_window = w->z_front; - } else { - w->z_back->z_front = w->z_front; - } - - w->z_front = w->z_back = nullptr; -} - /** * On clicking on a window, make it the frontmost window of all windows with an equal * or lower z-priority. The window is marked dirty for a repaint * @param w window that is put into the relative foreground + * @param dirty whether to mark the window dirty */ -static void BringWindowToFront(Window *w) +static void BringWindowToFront(Window *w, bool dirty) { - RemoveWindowFromZOrdering(w); - AddWindowToZOrdering(w); + auto priority = GetWindowZPriority(w->window_class); + WindowList::iterator dest = _z_windows.begin(); + while (dest != _z_windows.end() && (*dest == nullptr || GetWindowZPriority((*dest)->window_class) <= priority)) ++dest; - w->SetDirty(); + if (dest != w->z_position) { + _z_windows.splice(dest, _z_windows, w->z_position); + } + + if (dirty) w->SetDirty(); } /** @@ -1476,7 +1398,7 @@ void Window::InitializeData(WindowNumber window_number) if (!EditBoxInGlobalFocus() || this->nested_root->GetWidgetOfType(WWT_EDITBOX) != nullptr) SetFocusedWindow(this); /* Insert the window into the correct location in the z-ordering. */ - AddWindowToZOrdering(this); + BringWindowToFront(this, false); } /** @@ -1848,6 +1770,7 @@ void Window::InitNested(WindowNumber window_number) */ Window::Window(WindowDesc *desc) : window_desc(desc), mouse_capture_widget(-1) { + this->z_position = _z_windows.insert(_z_windows.end(), this); } /** @@ -1875,8 +1798,6 @@ void InitWindowSystem() { IConsoleClose(); - _z_back_window = nullptr; - _z_front_window = nullptr; _focused_window = nullptr; _mouseover_last_w = nullptr; _last_scroll_window = nullptr; @@ -1898,14 +1819,7 @@ void UnInitWindowSystem() for (Window *w : Window::Iterate()) delete w; - for (Window *w = _z_front_window; w != nullptr; /* nothing */) { - Window *to_del = w; - w = w->z_back; - free(to_del); - } - - _z_front_window = nullptr; - _z_back_window = nullptr; + _z_windows.clear(); } /** @@ -3068,15 +2982,13 @@ void InputLoop() CheckSoftLimit(); - /* Do the actual free of the deleted windows. */ - for (Window *v = _z_front_window; v != nullptr; /* nothing */) { - Window *w = v; - v = v->z_back; - - if (w->window_class != WC_INVALID) continue; - - RemoveWindowFromZOrdering(w); - free(w); + /* Remove dead entries from the window list */ + for (auto it = _z_windows.begin(); it != _z_windows.end(); ) { + if (*it == nullptr) { + it = _z_windows.erase(it); + } else { + ++it; + } } if (_input_events_this_tick != 0) { diff --git a/src/window_gui.h b/src/window_gui.h index 2b7c69367..1195c6c49 100644 --- a/src/window_gui.h +++ b/src/window_gui.h @@ -10,6 +10,8 @@ #ifndef WINDOW_GUI_H #define WINDOW_GUI_H +#include + #include "vehicle_type.h" #include "viewport_type.h" #include "company_type.h" @@ -143,8 +145,8 @@ void DrawFrameRect(int left, int top, int right, int bottom, Colours colour, Fra void DrawCaption(const Rect &r, Colours colour, Owner owner, TextColour text_colour, StringID str, StringAlignment align); /* window.cpp */ -extern Window *_z_front_window; -extern Window *_z_back_window; +using WindowList = std::list; +extern WindowList _z_windows; extern Window *_focused_window; @@ -293,19 +295,7 @@ public: * to destruct them all at the same time too, which is kinda hard. * @param size the amount of space not to allocate */ - inline void *operator new[](size_t size) - { - NOT_REACHED(); - } - - /** - * Helper allocation function to disallow something. - * Don't free the window directly; it corrupts the linked list when iterating - * @param ptr the pointer not to free - */ - inline void operator delete(void *ptr) - { - } + inline void *operator new[](size_t size) = delete; WindowDesc *window_desc; ///< Window description WindowFlags flags; ///< Window flags @@ -336,8 +326,7 @@ public: int mouse_capture_widget; ///< Widgetindex of current mouse capture widget (e.g. dragged scrollbar). -1 if no widget has mouse capture. Window *parent; ///< Parent window. - Window *z_front; ///< The window in front of us in z-order. - Window *z_back; ///< The window behind us in z-order. + WindowList::iterator z_position; template inline const NWID *GetWidget(uint widnum) const; @@ -813,9 +802,9 @@ public: /** * Iterator to iterate all valid Windows - * @tparam Tfront Wether we iterate from front + * @tparam TtoBack whether we iterate towards the back. */ - template + template struct WindowIterator { typedef Window *value_type; typedef value_type *pointer; @@ -823,22 +812,35 @@ public: typedef size_t difference_type; typedef std::forward_iterator_tag iterator_category; - explicit WindowIterator(const Window *start) : w(const_cast(start)) + explicit WindowIterator(WindowList::iterator start) : it(start) { this->Validate(); } + explicit WindowIterator(const Window *w) : it(w->z_position) {} - bool operator==(const WindowIterator &other) const { return this->w == other.w; } + bool operator==(const WindowIterator &other) const { return this->it == other.it; } bool operator!=(const WindowIterator &other) const { return !(*this == other); } - Window * operator*() const { return this->w; } + Window * operator*() const { return *this->it; } WindowIterator & operator++() { this->Next(); this->Validate(); return *this; } - bool IsEnd() const { return this->w == nullptr; } + bool IsEnd() const { return this->it == _z_windows.end(); } private: - Window *w; - void Validate() { while (this->w != nullptr && this->w->window_class == WC_INVALID) this->Next(); } - void Next() { if (this->w != nullptr) this->w = Tfront ? this->w->z_back : this->w->z_front; } + WindowList::iterator it; + void Validate() + { + while (!this->IsEnd() && *this->it == nullptr) this->Next(); + } + void Next() + { + if constexpr (!TtoBack) { + ++this->it; + } else if (this->it == _z_windows.begin()) { + this->it = _z_windows.end(); + } else { + --this->it; + } + } }; using IteratorToFront = WindowIterator; //!< Iterate in Z order towards front. using IteratorToBack = WindowIterator; //!< Iterate in Z order towards back. @@ -850,8 +852,17 @@ public: template struct AllWindows { AllWindows() {} - WindowIterator begin() { return WindowIterator(Tfront ? _z_front_window : _z_back_window); } - WindowIterator end() { return WindowIterator(nullptr); } + WindowIterator begin() + { + if constexpr (Tfront) { + auto back = _z_windows.end(); + if (back != _z_windows.begin()) --back; + return WindowIterator(back); + } else { + return WindowIterator(_z_windows.begin()); + } + } + WindowIterator end() { return WindowIterator(_z_windows.end()); } }; using Iterate = AllWindows; //!< Iterate all windows in whatever order is easiest. using IterateFromBack = AllWindows; //!< Iterate all windows in Z order from back to front. -- cgit v1.2.3-70-g09d2