[Spice-devel] Review of the C++ patches

[Frediano Ziglio]

>
> commit 43c6bf91b7c53ee9f93f7ea1cead5bba94c61f88
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Thu Mar 5 13:01:27 2020 +0000
>
>   reds: Remove a weak pointer usage
>
>   RedCharDevice can all be removed just calling unref, beside
>   the agent that needs special threatment.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> The logic of the code before/after the patch is not fully clear to me.
>
> I understand that the clean-up of `RedCharDevice`-based objects is
> simplified thanks to inheritance (`smartcard_device_disconnect` and
> `spicevmc_device_disconnect`'s behavior is now identical, so the
> cleanup is done in `reds_remove_char_device`.
>
> Seems fine to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

It's a pretty "standard" way to implement weak/strong references.
I think it's the same used by standard C++. It uses 2 counters, one
for weak pointers, the other for strong ones.

>
> commit ab7486a9e8667160390811abc677dfdc5ee33028
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Tue Mar 17 19:11:18 2020 +0000
>
>   main-channel-client: Automatically convert
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch converts C functions to C++ methods, and updates the code
> accordingly.
>
> Looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

The "Automatic" patches are almost helped by a series of really ugly scripts
(a mix of Coccinelle, Python, Bash and Perl!).

>
> commit fe0298a2905121a02ee64f429e9f88148d17c6ee
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Fri Mar 6 04:03:24 2020 +0000
>
>   safe-list: Add a class to implement a list with safe iterators
>
>   The reason to not using STL is that our code from how was designed requires
>   the iterator to be safe to the delete of the element pointed by the iterator.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch defines a `safe_list` class. As far as I understand (and as
> the name/commit/comments suggest), this class allows the deletion of
> the current iterator, while traversing the list.
>
>     /* Implementation of a list with more "safe" iterators.
>      * Specifically the item under an iterator can be removed while scanning
>      * the list. This to allow objects in the list to delete themselves from
>      * the list.
>      */
>
> I'm not a C++ export, but what I can read looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Unfortunately the code was and is relaying on this behaviour.
Better safe than sorry!

>
> commit 767a9caded058fbde64fa4cc8e719c6ccef5f707
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Fri Mar 6 04:13:00 2020 +0000
>
>   Allow to compile without C++ library
>
>   Provide a suitable allocator using GLib
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch defines a class-wrapper `Mallocator` around GLib
> `g_malloc/g_free` functions. This class is used with the `safe_list`
> class. The `==` and `!=` operators of the class are defined to return
> `true` / `false`, respectively.
>
> The purpose of this class and the operator overloading is unclear to
> me, but what I can read looks good.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Mainly the purpose is allowing to tell other container how to handle
memory allocations replacing default allocations.

>
> commit 54c083091943f61a8ebe0b4d420c3311c37df3e6
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Sun Mar 8 18:54:23 2020 +0000
>
>   input-channels: Improve encapsulation
>
>   Update member access to limit it.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch changes the visibility of multiple methods.
>
> Seems safe to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Yes, I think some warnings are raised if you don't have these
changes.

>
> commit 597461e443962fa0294794b1db3d2f1c0fb18812
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Sun Mar 8 05:46:56 2020 +0000
>
>   Add and use red::make_shared
>
>   Allow to create an object already contained in a shared pointer
>   to avoid having not owned objects.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch uses more `share_ptr` pointer wrappers.
>
> Minor: seems that some TODOs are left in the code:
>
>     // XXX make_shared
>
> Seems good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Yes, there are a bit of "XXX", but none are regressions.

> 
> On Mon, Jun 8, 2020 at 5:10 PM Kevin Pouget <kpouget at redhat.com> wrote:
> >
> > Hello spice-devel
> >
> > I worked on the review of Frediano's C++ patches during the last weeks,
> > I tried to understand the gist of the commits and summarize it in the
> > review message.
> > It's not an in-depth review, and I only spotted minor details.
> >
> > A few patches are not acked, mostly because they were too big for me to
> > study carefully enough.
> >
> > Overall, I really appreciate the effort made to adapt the code to C++, from
> > my point of view it is greatly beneficial for the code as it reduces a lot
> > the amount of code duplication (boilerplate code) by leveraging C++
> > inheritance, polymorphism, virtual methods, etc.
> > Likewise, the ref-counting is made simpler and safer with custom classes
> > (share_ptr). These custom classes mimic existing ones AFAICT, but they are
> > more "C-safe" as they cannot throw exceptions.
> >
> > 3 mails will follow with the reviews.
> >
> > Best regards,
> >
> > Kevin