This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 01/14] Introduce py-ref.h


Hi Tom,

On 11/12/2016 05:31 PM, Tom Tromey wrote:

> +
> +  /* Transfer ownership from OTHER.  */
> +  gdbpy_ref &operator= (gdbpy_ref &&other)
> +  {
> +    /* Note that this handles self-assignment properly.  */
> +    std::swap (m_obj, other.m_obj);

Since there's no need to worry about noexcept garantees here
(destructing a gdbpy_ref can't throw), I'd rather that
move would not be implemented as a swap, as that extends the
lifetime of m_obj in OTHER.

I.e., bugs like these will go unnoticed longer:

gdbpy_ref bar = make_whatever (....);
gdbpy_ref foo = make_whatever (....);

if (condition)
{
  bar = std::move (foo);
  some_function (foo.get ()); // whoops, passed bar.get () instead of NULL and crashing early.
}

I'd be happy with the idiom of implementing move-in-term-of-swap
by first move-constructing, and then swapping that temporary:

  gdbpy_ref &operator= (gdbpy_ref &&other)
  {
    gdbpy_ref (std::move (other)).swap (*this);
    return *this;
  }

Or perhaps simpler, go with something like what you had in the
first version:

+  /* Transfer ownership from another instance.  */
+  gdbpy_reference &operator= (gdbpy_reference &&other)
+  {
+    Py_XDECREF (m_obj);
+    m_obj = other.m_obj;
+    other.m_obj = NULL;
+    return *this;
+  }
+

But add a "if (this != &other)" check.  I'd tweak it to be
in terms of reset() while at it:

  /* Transfer ownership from another instance.  */
  gdbpy_reference &operator= (gdbpy_reference &&other)
  {
    if (this != &other)
      {
        reset (other.m_obj);
        other.m_obj = NULL;
      }
    return *this;
  }

Otherwise LGTM.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]