From 3028551b91263d4b1c01ef1c662ab2df0d97ebd7 Mon Sep 17 00:00:00 2001 From: elexis Date: Fri, 9 Aug 2019 17:25:55 +0000 Subject: [PATCH] MOVABLE idiom, const CGUI struct maps, in place move construction instead of copying temporaries during CGUI XML loading and GenerateText. Introduce MOVABLE idiom indicating that a class can use move semantics. Make values of CGUI struct maps holding XML data const to ensure at the root that the data is not modified. Use NONCOPYABLE and MOVABLE for SGUIIcon and SGUIStyle to enforce the non-copy policy on the compiler level (until someone changes the GUI design to make modifications needed). As indicated by that: Replace copy operations by in place move operations for these CGUI struct maps in the CGUI Xeromyces XML loading functions. Replace copy operations by const references for CSize and SGUIIcon in CGUIString::GenerateTextCall and CGUI::GenerateText. This avoids copying of non primitive members, such as the strings and containers of strings. Further related cleanup to be kept separated for auditability. Differential Revision: https://code.wildfiregames.com/D2163 Few comments on IRC by: wraitii, Itms Tested on: gcc 9, Jenkins, partially VS2015 Refs #1984, NONCOPYABLE CGUISpriteInstances: 0a7d0ecdde, 8f4f8e240f, c19f3608a5 NONCOPYABLE Image, Sprite: fb8032043b NONCOPYABLE GUI page: 94c57085e9 NONCOPYABLE GUIManager: 7c2e9027c2 NONCOPYABLE macro: 16ccae10cd This was SVN commit r22637. --- source/gui/CGUI.cpp | 22 +++++++++++++--------- source/gui/CGUI.h | 19 +++++++++++++------ source/gui/CGUISprite.cpp | 2 +- source/gui/CGUISprite.h | 10 ++++------ source/gui/GUIRenderer.cpp | 4 ++-- source/gui/GUIRenderer.h | 2 +- source/gui/GUIbase.h | 4 ++++ source/gui/GUItext.cpp | 4 ++-- source/gui/IGUIScrollBar.h | 5 +++++ source/lib/code_annotation.h | 8 ++++++++ 10 files changed, 53 insertions(+), 27 deletions(-) diff --git a/source/gui/CGUI.cpp b/source/gui/CGUI.cpp index c79bca61b9..2c3083b12f 100644 --- a/source/gui/CGUI.cpp +++ b/source/gui/CGUI.cpp @@ -383,7 +383,7 @@ void CGUI::Destroy() } m_pAllObjects.clear(); - for (const std::pair& p : m_Sprites) + for (const std::pair& p : m_Sprites) delete p.second; m_Sprites.clear(); m_Icons.clear(); @@ -478,7 +478,7 @@ void CGUI::SetFocusedObject(IGUIObject* pObject) const SGUIScrollBarStyle* CGUI::GetScrollBarStyle(const CStr& style) const { - std::map::const_iterator it = m_ScrollBarStyles.find(style); + std::map::const_iterator it = m_ScrollBarStyles.find(style); if (it == m_ScrollBarStyles.end()) return nullptr; @@ -594,9 +594,9 @@ SGUIText CGUI::GenerateText(const CGUIString& string, const CStrW& FontW, const _y = y; // Get Size from Icon database - SGUIIcon icon = GetIcon(imgname); + const SGUIIcon& icon = GetIcon(imgname); - CSize size = icon.m_Size; + const CSize& size = icon.m_Size; Image.SetupSpriteCall((j == CGUIString::SFeedback::Left), SpriteCall, Width, _y, size, icon.m_SpriteName, BufferZone, icon.m_CellID); // Check if image is the lowest thing. @@ -1423,7 +1423,8 @@ void CGUI::Xeromyces_ReadSprite(XMBElement Element, CXeromyces* pFile) delete effects; - m_Sprites[name] = Sprite; + m_Sprites.erase(name); + m_Sprites.emplace(name, Sprite); } void CGUI::Xeromyces_ReadImage(XMBElement Element, CXeromyces* pFile, CGUISprite& parent) @@ -1595,10 +1596,11 @@ void CGUI::Xeromyces_ReadStyle(XMBElement Element, CXeromyces* pFile) if (attr_name == "name") name = attr.Value; else - style.m_SettingsDefaults[attr_name] = attr.Value.FromUTF8(); + style.m_SettingsDefaults.emplace(attr_name, attr.Value.FromUTF8()); } - m_Styles[name] = style; + m_Styles.erase(name); + m_Styles.emplace(name, std::move(style)); } void CGUI::Xeromyces_ReadScrollBarStyle(XMBElement Element, CXeromyces* pFile) @@ -1680,7 +1682,8 @@ void CGUI::Xeromyces_ReadScrollBarStyle(XMBElement Element, CXeromyces* pFile) scrollbar.m_SpriteBarVerticalPressed = attr_value; } - m_ScrollBarStyles[name] = std::move(scrollbar); + m_ScrollBarStyles.erase(name); + m_ScrollBarStyles.emplace(name, std::move(scrollbar)); } void CGUI::Xeromyces_ReadIcon(XMBElement Element, CXeromyces* pFile) @@ -1720,7 +1723,8 @@ void CGUI::Xeromyces_ReadIcon(XMBElement Element, CXeromyces* pFile) debug_warn(L"Invalid data - DTD shouldn't allow this"); } - m_Icons[name] = icon; + m_Icons.erase(name); + m_Icons.emplace(name, std::move(icon)); } void CGUI::Xeromyces_ReadTooltip(XMBElement Element, CXeromyces* pFile) diff --git a/source/gui/CGUI.h b/source/gui/CGUI.h index 634becd7d1..8aa6f34a09 100644 --- a/source/gui/CGUI.h +++ b/source/gui/CGUI.h @@ -42,6 +42,11 @@ extern const double SELECT_DBLCLICK_RATE; */ struct SGUIStyle { + // Take advantage of moving the entire map instead and avoiding unintended copies. + NONCOPYABLE(SGUIStyle); + MOVABLE(SGUIStyle); + SGUIStyle() = default; + std::map m_SettingsDefaults; }; @@ -235,9 +240,9 @@ public: bool IconExists(const CStr& str) const { return (m_Icons.count(str) != 0); } /** - * Get Icon (a copy, can never be changed) + * Get Icon (a const reference, can never be changed) */ - SGUIIcon GetIcon(const CStr& str) const { return m_Icons.find(str)->second; } + const SGUIIcon& GetIcon(const CStr& str) const { return m_Icons.find(str)->second; } /** * Get pre-defined color (if it exists) @@ -628,19 +633,21 @@ private: //-------------------------------------------------------- // Databases + // These are loaded from XML files and marked as noncopyable and const to + // rule out unintentional modification and copy, especially during Draw calls. //-------------------------------------------------------- // Sprites - std::map m_Sprites; + std::map m_Sprites; // Styles - std::map m_Styles; + std::map m_Styles; // Scroll-bar styles - std::map m_ScrollBarStyles; + std::map m_ScrollBarStyles; // Icons - std::map m_Icons; + std::map m_Icons; }; #endif // INCLUDED_CGUI diff --git a/source/gui/CGUISprite.cpp b/source/gui/CGUISprite.cpp index 2e0638dd87..9a92fb4150 100644 --- a/source/gui/CGUISprite.cpp +++ b/source/gui/CGUISprite.cpp @@ -30,7 +30,7 @@ void CGUISprite::AddImage(SGUIImage* image) m_Images.push_back(image); } -void CGUISpriteInstance::Draw(const CRect& Size, int CellID, std::map& Sprites, float Z) const +void CGUISpriteInstance::Draw(const CRect& Size, int CellID, std::map& Sprites, float Z) const { if (m_CachedSize != Size || m_CachedCellID != CellID) { diff --git a/source/gui/CGUISprite.h b/source/gui/CGUISprite.h index 70e3baede4..249e62d3d9 100644 --- a/source/gui/CGUISprite.h +++ b/source/gui/CGUISprite.h @@ -157,16 +157,14 @@ public: // calculations between draw calls. class CGUISpriteInstance { - NONCOPYABLE(CGUISpriteInstance); - public: + NONCOPYABLE(CGUISpriteInstance); + MOVABLE(CGUISpriteInstance); + CGUISpriteInstance(); CGUISpriteInstance(const CStr& SpriteName); - CGUISpriteInstance(CGUISpriteInstance&&) = default; - CGUISpriteInstance& operator=(CGUISpriteInstance&&) = default; - - void Draw(const CRect& Size, int CellID, std::map& Sprites, float Z) const; + void Draw(const CRect& Size, int CellID, std::map& Sprites, float Z) const; bool IsEmpty() const; const CStr& GetName() const { return m_SpriteName; } void SetName(const CStr& SpriteName); diff --git a/source/gui/GUIRenderer.cpp b/source/gui/GUIRenderer.cpp index 33db956700..8bbf73af93 100644 --- a/source/gui/GUIRenderer.cpp +++ b/source/gui/GUIRenderer.cpp @@ -59,7 +59,7 @@ DrawCalls& DrawCalls::operator=(const DrawCalls&) } -void GUIRenderer::UpdateDrawCallCache(DrawCalls& Calls, const CStr& SpriteName, const CRect& Size, int CellID, std::map& Sprites) +void GUIRenderer::UpdateDrawCallCache(DrawCalls& Calls, const CStr& SpriteName, const CRect& Size, int CellID, std::map& Sprites) { // This is called only when something has changed (like the size of the // sprite), so it doesn't need to be particularly efficient. @@ -74,7 +74,7 @@ void GUIRenderer::UpdateDrawCallCache(DrawCalls& Calls, const CStr& SpriteName, return; - std::map::iterator it(Sprites.find(SpriteName)); + std::map::iterator it(Sprites.find(SpriteName)); if (it == Sprites.end()) { /* diff --git a/source/gui/GUIRenderer.h b/source/gui/GUIRenderer.h index c9fc3f5631..e33d5424a8 100644 --- a/source/gui/GUIRenderer.h +++ b/source/gui/GUIRenderer.h @@ -79,7 +79,7 @@ namespace GUIRenderer namespace GUIRenderer { - void UpdateDrawCallCache(DrawCalls& Calls, const CStr& SpriteName, const CRect& Size, int CellID, std::map& Sprites); + void UpdateDrawCallCache(DrawCalls& Calls, const CStr& SpriteName, const CRect& Size, int CellID, std::map& Sprites); void Draw(DrawCalls& Calls, float Z); } diff --git a/source/gui/GUIbase.h b/source/gui/GUIbase.h index 49e4511103..6aa118d89d 100644 --- a/source/gui/GUIbase.h +++ b/source/gui/GUIbase.h @@ -148,6 +148,10 @@ typedef std::vector vector_pObjects; // you use them in text owned by different objects... Such as CText. struct SGUIIcon { + // This struct represents an immutable type, so ensure to avoid copying the strings. + NONCOPYABLE(SGUIIcon); + MOVABLE(SGUIIcon); + SGUIIcon() : m_CellID(0) {} // Sprite name of icon diff --git a/source/gui/GUItext.cpp b/source/gui/GUItext.cpp index d675230df6..fe5c524ce7 100644 --- a/source/gui/GUItext.cpp +++ b/source/gui/GUItext.cpp @@ -124,9 +124,9 @@ void CGUIString::GenerateTextCall(const CGUI* pGUI, SFeedback& Feedback, CStrInt SGUIText::SSpriteCall SpriteCall; // Get Icon from icon database in pGUI - SGUIIcon icon = pGUI->GetIcon(path); + const SGUIIcon& icon = pGUI->GetIcon(path); - CSize size = icon.m_Size; + const CSize& size = icon.m_Size; // append width, and make maximum height the height. Feedback.m_Size.cx += size.cx; diff --git a/source/gui/IGUIScrollBar.h b/source/gui/IGUIScrollBar.h index 65e1b01959..5f1d81f9b1 100644 --- a/source/gui/IGUIScrollBar.h +++ b/source/gui/IGUIScrollBar.h @@ -49,6 +49,11 @@ A GUI ScrollBar */ struct SGUIScrollBarStyle { + // CGUISpriteInstance makes this NONCOPYABLE implicitly, make it explicit + NONCOPYABLE(SGUIScrollBarStyle); + MOVABLE(SGUIScrollBarStyle); + SGUIScrollBarStyle() = default; + //-------------------------------------------------------- /** @name General Settings */ //-------------------------------------------------------- diff --git a/source/lib/code_annotation.h b/source/lib/code_annotation.h index f5c2118d42..8a2c2e83b2 100644 --- a/source/lib/code_annotation.h +++ b/source/lib/code_annotation.h @@ -218,6 +218,14 @@ switch(x % 2) className(const className&) = delete; \ className& operator=(const className&) = delete +/** + * Indicates that move semantics can be used, so that a NONCOPYABLE class can still be assigned by taking over the reference to the value. + * Make sure to use the macro with the necessary access modifier. + */ +#define MOVABLE(className) \ + className(className&&) = default; \ + className& operator=(className&&) = default + #if ICC_VERSION # define ASSUME_ALIGNED(ptr, multiple) __assume_aligned(ptr, multiple) #else