[Libreoffice-commits] core.git: vcl/inc vcl/unx

[Stephan Bergmann]

 vcl/inc/unx/saldisp.hxx         |   24 +++++++++++++++++++++++-
 vcl/unx/generic/app/saldisp.cxx |   22 +++++++++++-----------
 2 files changed, 34 insertions(+), 12 deletions(-)

New commits:
commit a1f5b1f30a5a80d7eeed2efe2564763bf1d12086
Author: Stephan Bergmann <sbergman at redhat.com>
Date:   Thu Jun 28 09:47:29 2018 +0200

    Clean up ownership tracking of SalVisual's visual (in X11 Visual base class)
    
    GCC trunk towards GCC 9 emits -Wdeprecated-copy because SalVisual has implicit
    copy functions but a user-declared dtor.  The implicitly-defined copy functions
    are actually used (so cannot be deleted), but in fragile interaction with the
    semantics of the user-provided dtor in the SalColormap(sal_uInt16) ctor.
    
    Changing SalVisual into a move-only class (moving the "this instance must delete
    its visual member" information) doesn't work well, as SalDisplay::ScreenData is
    used as a supply of SalVisual instances (but which will never be of the "this
    instance must delete its visual member" variety) that are then copied into
    SalColormap::m_aVisual.
    
    As only SalColormap creates SalVisual instances of the "this instance must
    delete its visual member" variety, what actually works is to track that
    information in SalColormap instead of directly in SalVisual, and make
    SalColormap a move-only class.
    
    Change-Id: Ib968a38c40b660ce91981b3c7b281f4add6ece6b
    Reviewed-on: https://gerrit.libreoffice.org/56579
    Tested-by: Jenkins
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/vcl/inc/unx/saldisp.hxx b/vcl/inc/unx/saldisp.hxx
index a53be3692d8b..0bef7afb6c52 100644
--- a/vcl/inc/unx/saldisp.hxx
+++ b/vcl/inc/unx/saldisp.hxx
@@ -38,6 +38,7 @@ class   SalXLib;
 #include <vcl/ptrstyle.hxx>
 #include <sal/types.h>
 #include <osl/mutex.h>
+#include <cassert>
 #include <list>
 #include <unordered_map>
 #include <vector>
@@ -89,7 +90,6 @@ class SalVisual : public XVisualInfo
     int             nBlueBits_;
 public:
                             SalVisual();
-                            ~SalVisual();
                             SalVisual( const XVisualInfo* pXVI );
 
     VisualID        GetVisualId() const { return visualid; }
@@ -101,12 +101,29 @@ public:
             Color           GetTCColor( Pixel nPixel ) const;
 };
 
+// A move-only flag, used by SalColormap to track ownership of its m_aVisual.visual:
+struct OwnershipFlag {
+    bool owner = false;
+
+    OwnershipFlag() = default;
+
+    OwnershipFlag(OwnershipFlag && other) noexcept: owner(other.owner) { other.owner = false; }
+
+    OwnershipFlag & operator =(OwnershipFlag && other) noexcept {
+        assert(&other != this);
+        owner = other.owner;
+        other.owner = false;
+        return *this;
+    }
+};
+
 class SalColormap
 {
     const SalDisplay*       m_pDisplay;
     Colormap                m_hColormap;
     std::vector<Color>      m_aPalette;         // Pseudocolor
     SalVisual               m_aVisual;
+    OwnershipFlag           m_aVisualOwnership;
     std::vector<sal_uInt16>     m_aLookupTable;     // Pseudocolor: 12bit reduction
     Pixel                   m_nWhitePixel;
     Pixel                   m_nBlackPixel;
@@ -122,6 +139,11 @@ public:
     SalColormap( sal_uInt16         nDepth );
     SalColormap();
 
+    ~SalColormap();
+
+    SalColormap(SalColormap &&) = default;
+    SalColormap & operator =(SalColormap &&) = default;
+
     Colormap            GetXColormap() const { return m_hColormap; }
     const SalDisplay*   GetDisplay() const { return m_pDisplay; }
     inline  Display*            GetXDisplay() const;
diff --git a/vcl/unx/generic/app/saldisp.cxx b/vcl/unx/generic/app/saldisp.cxx
index 5d266828196a..dff49c8e8b7c 100644
--- a/vcl/unx/generic/app/saldisp.cxx
+++ b/vcl/unx/generic/app/saldisp.cxx
@@ -2435,11 +2435,6 @@ SalVisual::SalVisual( const XVisualInfo* pXVI )
     }
 }
 
-SalVisual::~SalVisual()
-{
-    if( -1 == screen && VisualID(-1) == visualid ) delete visual;
-}
-
 // Converts the order of bytes of a Pixel into bytes of a Color
 // This is not reversible for the 6 XXXA
 
@@ -2608,8 +2603,8 @@ SalColormap::SalColormap( sal_uInt16 nDepth )
                                &aVI ) )
         {
             aVI.visual          = new Visual;
-            aVI.visualid        = VisualID(0); // beware of temporary destructor below
-            aVI.screen          = 0;
+            aVI.visualid        = VisualID(-1);
+            aVI.screen          = -1;
             aVI.depth           = nDepth;
             aVI.c_class         = TrueColor;
             if( 24 == nDepth ) // 888
@@ -2661,16 +2656,21 @@ SalColormap::SalColormap( sal_uInt16 nDepth )
             aVI.visual->map_entries     = aVI.colormap_size;
 
             m_aVisual = SalVisual( &aVI );
-            // give ownership of constructed Visual() to m_aVisual
-            // see SalVisual destructor
-            m_aVisual.visualid        = VisualID(-1);
-            m_aVisual.screen          = -1;
+            m_aVisualOwnership.owner = true;
         }
         else
             m_aVisual = SalVisual( &aVI );
     }
 }
 
+SalColormap::~SalColormap()
+{
+    if (m_aVisualOwnership.owner)
+    {
+        delete m_aVisual.visual;
+    }
+}
+
 void SalColormap::GetPalette()
 {
     Pixel i;