> > commit 43c6bf91b7c53ee9f93f7ea1cead5bba94c61f88 > Author: Frediano Ziglio <fziglio@xxxxxxxxxx> > 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> 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@xxxxxxxxxx> > Date: Tue Mar 17 19:11:18 2020 +0000 > > main-channel-client: Automatically convert > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > > This patch converts C functions to C++ methods, and updates the code > accordingly. > > Looks good to me. > > Acked-by: Kevin Pouget <kpouget@xxxxxxxxxx> 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@xxxxxxxxxx> > 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> Unfortunately the code was and is relaying on this behaviour. Better safe than sorry! > > commit 767a9caded058fbde64fa4cc8e719c6ccef5f707 > Author: Frediano Ziglio <fziglio@xxxxxxxxxx> > 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> Mainly the purpose is allowing to tell other container how to handle memory allocations replacing default allocations. > > commit 54c083091943f61a8ebe0b4d420c3311c37df3e6 > Author: Frediano Ziglio <fziglio@xxxxxxxxxx> > 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@xxxxxxxxxx> > > This patch changes the visibility of multiple methods. > > Seems safe to me. > > Acked-by: Kevin Pouget <kpouget@xxxxxxxxxx> Yes, I think some warnings are raised if you don't have these changes. > > commit 597461e443962fa0294794b1db3d2f1c0fb18812 > Author: Frediano Ziglio <fziglio@xxxxxxxxxx> > 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> Yes, there are a bit of "XXX", but none are regressions. > > On Mon, Jun 8, 2020 at 5:10 PM Kevin Pouget <kpouget@xxxxxxxxxx> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel