This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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