From a905fbbc98d4a5d33c6988fb78cc97e48124c801 Mon Sep 17 00:00:00 2001 From: elexis Date: Sat, 10 Aug 2019 00:04:17 +0000 Subject: [PATCH] Use NONCOPYABLE for most GUI classes and structs to have the compiler indicate unintended copies, refs 3028551b91 / D2163. That is CChartData, CGUIList, CGUISeries, COListColumn, GUITooltip, SGUIMessage, SSpriteCall, STextCall, SFeedback, IGUISetting, CGUISetting, GUI, IGUIObject, IGUIScrollBar. Drop copying GetSetting and SetSetting template functions for CGUIList, CGUISeries, CClientArea, CGUIString. Stop copying COListColumn. Drop copying GUI::GetSetting call in IGUIObject::UpdateCachedSize() and four copying GUI::GetSetting calls in SetupText() functions. Delete unused GUIRenderer IGLState class from 849f50a500 obsolete since 1f5b8f1c9a. Differential Revision: https://code.wildfiregames.com/D2164 This was SVN commit r22638. --- source/gui/CButton.cpp | 6 +++--- source/gui/CChart.h | 5 +++++ source/gui/CCheckBox.cpp | 6 +++--- source/gui/CGUI.cpp | 6 ++++-- source/gui/CGUIList.h | 8 +++++++- source/gui/CGUISeries.h | 8 +++++--- source/gui/COList.cpp | 10 ++++------ source/gui/COList.h | 12 ++++++++---- source/gui/CText.cpp | 7 ++++--- source/gui/CTooltip.cpp | 6 +++--- source/gui/GUIManager.cpp | 2 +- source/gui/GUIManager.h | 2 ++ source/gui/GUIRenderer.h | 8 -------- source/gui/GUITooltip.h | 4 +++- source/gui/GUIbase.h | 5 +++++ source/gui/GUItext.cpp | 4 ++-- source/gui/GUItext.h | 12 ++++++++++++ source/gui/GUItypes.h | 13 ++++++++----- source/gui/GUIutil.cpp | 21 ++++++++++----------- source/gui/GUIutil.h | 8 +++++++- source/gui/IGUIObject.cpp | 8 ++++---- source/gui/IGUIObject.h | 2 ++ source/gui/IGUIScrollBar.h | 2 ++ source/gui/MiniMap.cpp | 1 + 24 files changed, 105 insertions(+), 61 deletions(-) diff --git a/source/gui/CButton.cpp b/source/gui/CButton.cpp index f53120d429..4cd372767a 100644 --- a/source/gui/CButton.cpp +++ b/source/gui/CButton.cpp @@ -68,12 +68,12 @@ void CButton::SetupText() // TODO Gee: (2004-08-14) Default should not be hard-coded, but be in styles! font = L"default"; - CGUIString caption; - GUI::GetSetting(this, "caption", caption); + CGUIString* caption = nullptr; + GUI::GetSettingPointer(this, "caption", caption); float buffer_zone = 0.f; GUI::GetSetting(this, "buffer_zone", buffer_zone); - *m_GeneratedTexts[0] = GetGUI()->GenerateText(caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this); + *m_GeneratedTexts[0] = GetGUI()->GenerateText(*caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this); CalculateTextPosition(m_CachedActualSize, m_TextPos, *m_GeneratedTexts[0]); } diff --git a/source/gui/CChart.h b/source/gui/CChart.h index 3dbd2ad57c..5d32eb5b75 100644 --- a/source/gui/CChart.h +++ b/source/gui/CChart.h @@ -27,6 +27,11 @@ struct CChartData { + // Avoid copying the container. + NONCOPYABLE(CChartData); + MOVABLE(CChartData); + CChartData() = default; + CGUIColor m_Color; std::vector m_Points; }; diff --git a/source/gui/CCheckBox.cpp b/source/gui/CCheckBox.cpp index 93e3b35bd5..cf87763f91 100644 --- a/source/gui/CCheckBox.cpp +++ b/source/gui/CCheckBox.cpp @@ -80,12 +80,12 @@ void CCheckBox::SetupText() float square_side; GUI::GetSetting(this, "square_side", square_side); - CGUIString caption; - GUI::GetSetting(this, "caption", caption); + CGUIString* caption = nullptr; + GUI::GetSettingPointer(this, "caption", caption); float buffer_zone = 0.f; GUI::GetSetting(this, "buffer_zone", buffer_zone); - *m_GeneratedTexts[0] = GetGUI()->GenerateText(caption, font, m_CachedActualSize.GetWidth()-square_side, 0.f, this); + *m_GeneratedTexts[0] = GetGUI()->GenerateText(*caption, font, m_CachedActualSize.GetWidth()-square_side, 0.f, this); } void CCheckBox::HandleMessage(SGUIMessage& Message) diff --git a/source/gui/CGUI.cpp b/source/gui/CGUI.cpp index 2c3083b12f..dfd5813f69 100644 --- a/source/gui/CGUI.cpp +++ b/source/gui/CGUI.cpp @@ -488,6 +488,8 @@ const SGUIScrollBarStyle* CGUI::GetScrollBarStyle(const CStr& style) const // private struct used only in GenerateText(...) struct SGenerateTextImage { + // Only primitve members, so move assignments are the same as copy assignments. + float m_YFrom, // The image's starting location in Y m_YTo, // The image's end location in Y m_Indentation; // The image width in other words @@ -602,8 +604,8 @@ SGUIText CGUI::GenerateText(const CGUIString& string, const CStrW& FontW, const // Check if image is the lowest thing. Text.m_Size.cy = std::max(Text.m_Size.cy, Image.m_YTo); - Images[j].push_back(Image); - Text.m_SpriteCalls.push_back(std::move(SpriteCall)); + Images[j].emplace_back(Image); + Text.m_SpriteCalls.emplace_back(std::move(SpriteCall)); } } } diff --git a/source/gui/CGUIList.h b/source/gui/CGUIList.h index 33578392db..ff8d595091 100644 --- a/source/gui/CGUIList.h +++ b/source/gui/CGUIList.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2009 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -26,6 +26,12 @@ class CGUIList public: // struct:ish (but for consistency I call it _C_GUIList, and // for the same reason it is a class, so that confusion doesn't // appear when forward declaring. + + // Avoid copying the vector. + NONCOPYABLE(CGUIList); + MOVABLE(CGUIList); + CGUIList() = default; + /** * List of items (as text), the post-processed result is stored in * the IGUITextOwner structure of this class. diff --git a/source/gui/CGUISeries.h b/source/gui/CGUISeries.h index fe43ba4043..05ce6243ae 100644 --- a/source/gui/CGUISeries.h +++ b/source/gui/CGUISeries.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2017 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -19,13 +19,15 @@ #ifndef INCLUDED_CGUISERIES #define INCLUDED_CGUISERIES -#include "GUItext.h" #include "maths/Vector2D.h" - class CGUISeries { public: + NONCOPYABLE(CGUISeries); + MOVABLE(CGUISeries); + CGUISeries() = default; + std::vector> m_Series; }; diff --git a/source/gui/COList.cpp b/source/gui/COList.cpp index 24feac3e34..5109cafa6e 100644 --- a/source/gui/COList.cpp +++ b/source/gui/COList.cpp @@ -297,20 +297,18 @@ bool COList::HandleAdditionalChildren(const XMBElement& child, CXeromyces* pFile } } - m_Columns.push_back(column); - AddSetting("list_" + column.m_Id); AddSetting("hidden_" + column.m_Id); GUI::SetSetting(this, "hidden_" + column.m_Id, hidden); + m_Columns.emplace_back(std::move(column)); + SetupText(); return true; } - else - { - return false; - } + + return false; } void COList::DrawList(const int& selected, const CStr& _sprite, const CStr& _sprite_selected, const CStr& _textcolor) diff --git a/source/gui/COList.h b/source/gui/COList.h index ebafbe1cda..56785a1d21 100644 --- a/source/gui/COList.h +++ b/source/gui/COList.h @@ -25,11 +25,15 @@ */ struct COListColumn { - CGUIColor m_TextColor; - CStr m_Id; - float m_Width; - CStrW m_Heading; + // Avoid copying the strings. + NONCOPYABLE(COListColumn); + MOVABLE(COListColumn); + COListColumn() = default; + CGUIColor m_TextColor; + CStr m_Id; + float m_Width; + CStrW m_Heading; }; /** diff --git a/source/gui/CText.cpp b/source/gui/CText.cpp index 7e072cc6c7..e2e6d98b0e 100644 --- a/source/gui/CText.cpp +++ b/source/gui/CText.cpp @@ -75,9 +75,10 @@ void CText::SetupText() // TODO Gee: (2004-08-14) Don't define standard like this. Do it with the default style. font = L"default"; - CGUIString caption; + CGUIString* caption = nullptr; + GUI::GetSettingPointer(this, "caption", caption); + bool scrollbar; - GUI::GetSetting(this, "caption", caption); GUI::GetSetting(this, "scrollbar", scrollbar); float width = m_CachedActualSize.GetWidth(); @@ -88,7 +89,7 @@ void CText::SetupText() float buffer_zone = 0.f; GUI::GetSetting(this, "buffer_zone", buffer_zone); - *m_GeneratedTexts[0] = GetGUI()->GenerateText(caption, font, width, buffer_zone, this); + *m_GeneratedTexts[0] = GetGUI()->GenerateText(*caption, font, width, buffer_zone, this); if (!scrollbar) CalculateTextPosition(m_CachedActualSize, m_TextPos, *m_GeneratedTexts[0]); diff --git a/source/gui/CTooltip.cpp b/source/gui/CTooltip.cpp index f68e4971d1..f3ed6801c1 100644 --- a/source/gui/CTooltip.cpp +++ b/source/gui/CTooltip.cpp @@ -75,13 +75,13 @@ void CTooltip::SetupText() float buffer_zone = 0.f; GUI::GetSetting(this, "buffer_zone", buffer_zone); - CGUIString caption; - GUI::GetSetting(this, "caption", caption); + CGUIString* caption = nullptr; + GUI::GetSettingPointer(this, "caption", caption); float max_width = 0.f; GUI::GetSetting(this, "maxwidth", max_width); - *m_GeneratedTexts[0] = GetGUI()->GenerateText(caption, font, max_width, buffer_zone, this); + *m_GeneratedTexts[0] = GetGUI()->GenerateText(*caption, font, max_width, buffer_zone, this); // Position the tooltip relative to the mouse: diff --git a/source/gui/GUIManager.cpp b/source/gui/GUIManager.cpp index 23db228095..d500588d72 100644 --- a/source/gui/GUIManager.cpp +++ b/source/gui/GUIManager.cpp @@ -96,7 +96,7 @@ void CGUIManager::PushPage(const CStrW& pageName, shared_ptr initData); void LoadPage(shared_ptr scriptRuntime); diff --git a/source/gui/GUIRenderer.h b/source/gui/GUIRenderer.h index e33d5424a8..4233d64b44 100644 --- a/source/gui/GUIRenderer.h +++ b/source/gui/GUIRenderer.h @@ -32,14 +32,6 @@ struct SGUIImage; namespace GUIRenderer { - class IGLState - { - public: - virtual ~IGLState() {}; - virtual void Set(const CTexturePtr& tex) = 0; - virtual void Unset() = 0; - }; - struct SDrawCall { SDrawCall(const SGUIImage* image) : m_Image(image) {} diff --git a/source/gui/GUITooltip.h b/source/gui/GUITooltip.h index c02170539a..cdaae6b944 100644 --- a/source/gui/GUITooltip.h +++ b/source/gui/GUITooltip.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2015 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -27,6 +27,8 @@ class CGUI; class GUITooltip { public: + NONCOPYABLE(GUITooltip); + GUITooltip(); void Update(IGUIObject* Nearest, const CPos& MousePos, CGUI* GUI); diff --git a/source/gui/GUIbase.h b/source/gui/GUIbase.h index 6aa118d89d..2c3a5f4709 100644 --- a/source/gui/GUIbase.h +++ b/source/gui/GUIbase.h @@ -95,6 +95,9 @@ enum EGUIMessageType */ struct SGUIMessage { + // This should be passed as a const reference or pointer. + NONCOPYABLE(SGUIMessage); + SGUIMessage(EGUIMessageType _type) : type(_type), skipped(false) {} SGUIMessage(EGUIMessageType _type, const CStr& _value) : type(_type), value(_value), skipped(false) {} @@ -173,6 +176,8 @@ struct SGUIIcon class CClientArea { public: + // COPYABLE, since there are only primitives involved, making move and copy identical, + // and since some temporaries cannot be avoided. CClientArea(); CClientArea(const CStr& Value); CClientArea(const CRect& pixel, const CRect& percent); diff --git a/source/gui/GUItext.cpp b/source/gui/GUItext.cpp index fe5c524ce7..490421d859 100644 --- a/source/gui/GUItext.cpp +++ b/source/gui/GUItext.cpp @@ -165,7 +165,7 @@ void CGUIString::GenerateTextCall(const CGUI* pGUI, SFeedback& Feedback, CStrInt TextCall.m_pSpriteCall = &Feedback.m_SpriteCalls.back(); // Add text call - Feedback.m_TextCalls.push_back(TextCall); + Feedback.m_TextCalls.emplace_back(std::move(TextCall)); break; } @@ -227,7 +227,7 @@ void CGUIString::GenerateTextCall(const CGUI* pGUI, SFeedback& Feedback, CStrInt Feedback.m_NewLine = true; // Add text-chunk - Feedback.m_TextCalls.push_back(TextCall); + Feedback.m_TextCalls.emplace_back(std::move(TextCall)); } } } diff --git a/source/gui/GUItext.h b/source/gui/GUItext.h index 214e3b5ce7..f5e3b1ed09 100644 --- a/source/gui/GUItext.h +++ b/source/gui/GUItext.h @@ -60,6 +60,10 @@ struct SGUIText */ struct SSpriteCall { + // The CGUISpriteInstance makes this uncopyable to avoid invalidating its draw cache + NONCOPYABLE(SSpriteCall); + MOVABLE(SSpriteCall); + SSpriteCall() : m_CellID(0) {} /** @@ -90,6 +94,9 @@ struct SGUIText */ struct STextCall { + NONCOPYABLE(STextCall); + MOVABLE(STextCall); + STextCall() : m_UseCustomColor(false), m_Bold(false), m_Italic(false), m_Underlined(false), @@ -248,6 +255,11 @@ public: */ struct SFeedback { + // Avoid copying the vector and list containers. + NONCOPYABLE(SFeedback); + MOVABLE(SFeedback); + SFeedback() = default; + // Constants static const int Left = 0; static const int Right = 1; diff --git a/source/gui/GUItypes.h b/source/gui/GUItypes.h index af75d7b387..6f6c1ea92d 100644 --- a/source/gui/GUItypes.h +++ b/source/gui/GUItypes.h @@ -25,20 +25,23 @@ places, and to make it much easier to add a new type). Just do to handle every possible type. */ +#ifndef GUITYPE_IGNORE_COPYABLE TYPE(bool) TYPE(i32) TYPE(u32) TYPE(float) TYPE(CGUIColor) -TYPE(CClientArea) -TYPE(CGUIString) -#ifndef GUITYPE_IGNORE_CGUISpriteInstance -TYPE(CGUISpriteInstance) -#endif TYPE(CStr) TYPE(CStrW) TYPE(EAlign) TYPE(EVAlign) TYPE(CPos) +#endif + +#ifndef GUITYPE_IGNORE_NONCOPYABLE +TYPE(CClientArea) TYPE(CGUIList) TYPE(CGUISeries) +TYPE(CGUISpriteInstance) +TYPE(CGUIString) +#endif diff --git a/source/gui/GUIutil.cpp b/source/gui/GUIutil.cpp index b61999a1be..6359d77c4c 100644 --- a/source/gui/GUIutil.cpp +++ b/source/gui/GUIutil.cpp @@ -388,22 +388,21 @@ PSRETURN GUI::SetSettingWrap(IGUIObject* pObject, const CStr& Setting, const } // Instantiate templated functions: +// These functions avoid copies by working with a pointer and move semantics. #define TYPE(T) \ template PSRETURN GUI::GetSettingPointer(const IGUIObject* pObject, const CStr& Setting, T*& Value); \ - template PSRETURN GUI::GetSetting(const IGUIObject* pObject, const CStr& Setting, T& Value); \ template PSRETURN GUI::SetSetting(IGUIObject* pObject, const CStr& Setting, T& Value, const bool& SkipMessage); \ - template PSRETURN GUI::SetSetting(IGUIObject* pObject, const CStr& Setting, const T& Value, const bool& SkipMessage); \ template class CGUISetting; \ -#define GUITYPE_IGNORE_CGUISpriteInstance #include "GUItypes.h" -#undef GUITYPE_IGNORE_CGUISpriteInstance #undef TYPE -// Don't instantiate GetSetting - this will cause linker errors if -// you attempt to retrieve a sprite using GetSetting, since that copies the sprite -// and will mess up the caching performed by DrawSprite. You have to use GetSettingPointer -// instead. (This is mainly useful to stop me accidentally using the wrong function.) -template PSRETURN GUI::GetSettingPointer(const IGUIObject* pObject, const CStr& Setting, CGUISpriteInstance*& Value); -template PSRETURN GUI::SetSetting(IGUIObject* pObject, const CStr& Setting, CGUISpriteInstance& Value, const bool& SkipMessage); -template class CGUISetting; +// Copying functions - discouraged except for primitives. +#define TYPE(T) \ + template PSRETURN GUI::GetSetting(const IGUIObject* pObject, const CStr& Setting, T& Value); \ + template PSRETURN GUI::SetSetting(IGUIObject* pObject, const CStr& Setting, const T& Value, const bool& SkipMessage); \ + +#define GUITYPE_IGNORE_NONCOPYABLE +#include "GUItypes.h" +#undef GUITYPE_IGNORE_NONCOPYABLE +#undef TYPE diff --git a/source/gui/GUIutil.h b/source/gui/GUIutil.h index 72d382d9a2..aeb31fed5f 100644 --- a/source/gui/GUIutil.h +++ b/source/gui/GUIutil.h @@ -46,7 +46,10 @@ template class GUI; class IGUISetting { public: - virtual ~IGUISetting() {}; + NONCOPYABLE(IGUISetting); + + IGUISetting() = default; + virtual ~IGUISetting() = default; /** * Parses the given string and assigns to the setting value. Used for parsing XML attributes. @@ -70,6 +73,8 @@ class CGUISetting : public IGUISetting friend class GUI; public: + NONCOPYABLE(CGUISetting); + CGUISetting(IGUIObject& pObject, const CStr& Name); /** @@ -125,6 +130,7 @@ class GUI friend class IGUIObject; public: + NONCOPYABLE(GUI); // Like GetSetting (below), but doesn't make a copy of the value // (so it can be modified later) diff --git a/source/gui/IGUIObject.cpp b/source/gui/IGUIObject.cpp index 47a4831d2c..f723a97e5c 100644 --- a/source/gui/IGUIObject.cpp +++ b/source/gui/IGUIObject.cpp @@ -238,16 +238,16 @@ void IGUIObject::UpdateCachedSize() float aspectratio = 0.f; GUI::GetSetting(this, "aspectratio", aspectratio); - CClientArea ca; - GUI::GetSetting(this, "size", ca); + CClientArea* ca; + GUI::GetSettingPointer(this, "size", ca); // If absolute="false" and the object has got a parent, // use its cached size instead of the screen. Notice // it must have just been cached for it to work. if (absolute == false && m_pParent && !IsRootObject()) - m_CachedActualSize = ca.GetClientArea(m_pParent->m_CachedActualSize); + m_CachedActualSize = ca->GetClientArea(m_pParent->m_CachedActualSize); else - m_CachedActualSize = ca.GetClientArea(CRect(0.f, 0.f, g_xres / g_GuiScale, g_yres / g_GuiScale)); + m_CachedActualSize = ca->GetClientArea(CRect(0.f, 0.f, g_xres / g_GuiScale, g_yres / g_GuiScale)); // In a few cases, GUI objects have to resize to fill the screen // but maintain a constant aspect ratio. diff --git a/source/gui/IGUIObject.h b/source/gui/IGUIObject.h index 770df18583..33d22491b1 100644 --- a/source/gui/IGUIObject.h +++ b/source/gui/IGUIObject.h @@ -60,6 +60,8 @@ class IGUIObject friend bool JSI_IGUIObject::getComputedSize(JSContext* cx, uint argc, JS::Value* vp); public: + NONCOPYABLE(IGUIObject); + IGUIObject(CGUI* pGUI); virtual ~IGUIObject(); diff --git a/source/gui/IGUIScrollBar.h b/source/gui/IGUIScrollBar.h index 5f1d81f9b1..440cb47484 100644 --- a/source/gui/IGUIScrollBar.h +++ b/source/gui/IGUIScrollBar.h @@ -160,6 +160,8 @@ struct SGUIScrollBarStyle class IGUIScrollBar { public: + NONCOPYABLE(IGUIScrollBar); + IGUIScrollBar(CGUI* pGUI); virtual ~IGUIScrollBar(); diff --git a/source/gui/MiniMap.cpp b/source/gui/MiniMap.cpp index 8da62c44f4..ccf22ed09e 100644 --- a/source/gui/MiniMap.cpp +++ b/source/gui/MiniMap.cpp @@ -316,6 +316,7 @@ void CMiniMap::DrawViewRect(CMatrix3D transform) struct MinimapUnitVertex { + // This struct is copyable for convenience and because to move is to copy for primitives. u8 r, g, b, a; float x, y; };