Re: [lmi] [lmi-commits] master a67848a 1/3: Use 'new(wx)' to allocate memory that will be freed by wx

[Vadim Zeitlin]

On Sun, 7 Feb 2021 23:17:08 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 2/6/21 11:00 PM, Vadim Zeitlin wrote:
GC> > On Sat,  6 Feb 2021 14:47:06 -0500 (EST) Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> [...]
GC> > GC> diff --git a/skeleton.cpp b/skeleton.cpp
GC> > GC> index 18e4494..22e96d3 100644
GC> > GC> --- a/skeleton.cpp
GC> > GC> +++ b/skeleton.cpp
GC> > GC> @@ -1081,6 +1081,9 @@ void 
Skeleton::UponTestFloatingPointEnvironment(wxCommandEvent&)
GC> > GC>  
GC> > GC>  void Skeleton::UponTestPasting(wxCommandEvent&)
GC> > GC>  {
GC> > GC> +    // This uses 'new' rather than 'new(wx)' because the object is
GC> > GC> +    // explicitly deleted by calling Destroy(); yet the Destroy()
GC> > GC> +    // call isn't reached if an exception is thrown.
GC> > GC>      InputSequenceEntry* z = new InputSequenceEntry(frame_, wxID_ANY, 
"Testing...");
GC> > GC>      LMI_ASSERT(z);
GC> > GC>      wxTextCtrl& t = z->text_ctrl();
GC> 
GC> [...snip other comments already addressed, though not yet pushed...]
GC> 
GC> >  Independently of this, we probably ought to use some kind of scope guard
GC> > to ensure that Destroy() is called in any case, as even if there is no 
real
GC> > memory leak here (because the InputSequenceEntry will eventually be 
deleted
GC> > by the frame), there is a "window leak", as the entry window will remain
GC> > orphaned on the screen. Or we could just use a unique_ptr as Destroy() for
GC> > a child window is the same as deleting it anyhow.
GC> 
GC> Writing a scope guard seems like too much work for this test;
GC> unique_ptr sounds much better. But what would become a unique_ptr?
GC> The thing named 'z', of type InputSequenceEntry (derived from wxPanel)?

 Yes.

GC> Would we write
GC>   std::unique_ptr<InputSequenceEntry> z = new(wx) InputSequenceEntry(...);
GC> because there's no make_unique(wx) that would delegate to new(wx)?

 I could be wrong, but I thought we only need to use new(wx) for the
pointers deleted by wxWidgets. And normally this is the case for all window
pointers that we create in lmi code, which is why we use new(wx) for all of
them. However in this particular case we want to delete the window
ourselves, so it won't be deleted by wxWidgets and hence there is no need
for new(wx) and we can just write

        auto const z = std::make_unique<InputSequenceEntry>(...);

GC> And then would we just expunge this line:
GC>       z->Destroy();
GC> ?

 Yes.

GC> I just feel unsure mixing unique_ptr with wx.

 It's unusual, but there is no reason it shouldn't work.

 Also, we could use a custom deleter to preserve _exactly_ the semantics of
the existing code and just fix the bug happening in case of exception, i.e.

        auto const window_destroyer = [](wxWindow* w) { w->Destroy(); };
        std::unique_ptr<wxWindow, decltype(window_destroyer)>
            z{new InputSequenceEntry(...), window_destroyer};

(and still remove the line calling Destroy() manually, of course).

 Regards,
VZ

pgpCIOY0V0yb2.pgp
Description: PGP signature