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

Mon Jun 15 11:13:22 UTC 2020

> commit 7b3637417062fe657c0bf1fced5ef59a489dbdc3
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Mon Mar 9 10:04:24 2020 +0000
>
>   Adjust some warnings
>
>   Remove -Werror and add -fpermissive, this will allow to compile C code with
>   a GNU C++ compiler.
>
>   Ignore warnings as our code use some feature like empty arrays.
>
>   Remove warnings not available in C++.
>
>   Bump GLIB_VERSION_MAX_ALLOWED to reduce the warning, looks like the
>   GLib headers for C++ are not able to handle them correctly.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch adds multiple warning to the build systems.
>
> I understand that they ease the transition from C to 'C + C++', but
> without a deeper look it's hard to tell which ones should be
> eventually removed and which ones (if any) must stay because of the
> C/C++ cooperation.
>
> Minor: the explicit-cast fix in `server/red-client.c` doesn't seem to
> belong to this patch. I wonder if it's a patch split issue or if there
> is a link with build system patch.
>
>
> Apart from this minor comment, the patch looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Yes, could be a split issue. Not that changes much at the end of the series
but probably was a split.

>
> commit e788e6e4762fc1c9a13a44b05907fb109bef9cde
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Wed May 22 07:45:30 2019 +0100
>
>   Remove conversion warnings
>
>   Use casts to avoid all that warning.
>   They should go away once you use more type safe types.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch adds explicit casting in many (many many) places.
>
> Minor style inconsistency: pointer cast types are sometimes written `(type
> *)`, sometimes `(type*)`, and once `(type* )`.
>

It was a pity, sorry about that

> There is a type changed from `RedChannelClient *rcc` to
> `CursorChannelClient *ccc` in function
> `cursor_channel_client_reset_cursor_cache`. I'm not sure this belonged
> to this patch. But the change seems to be consistent, with `ccc =
> CURSOR_CHANNEL_CLIENT(rcc)`
>
>
> Besides that, the patch looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

thanks

>
> commit e6e6ded681ff154f236f260f071389c4c8c0d944
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Sun May 26 15:10:28 2019 +0100
>
>   Use C++ IS-A relationship for RedChannelClient and RedChannel
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
>
> The code modifications of this patch uses inheritance for structure
> types with a parent:
>
>     struct CommonGraphicsChannel
>     {
>         RedChannel parent;
>
>         ...
>     };
>
> becomes:
>
>     struct CommonGraphicsChannel: public RedChannel
>     {
>         CommonGraphicsChannelPrivate *priv;
>     };
>
>
> Minor: this patch renames 19 files from C to C++ without any
> modification, they could have been in a dedicated patch. Likewise,
> some modifications are purly C-to-C++ transition (eg,
> `s/inttypes.h/ctypes/`).
>
> Minor: some of the modifications could have been included in the
> previous patch (casts) or elsewhere (eg, assert-before-ref check).
>
> Besides that, the patch looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

Probably the files included some headers that requires C++.

>
> commit 874e7450887375730a4b3675b9b1b7002b440ea8
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Tue May 28 05:03:01 2019 +0100
>
>   Move all red_channel_client_* functions in header as methods
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This (big) patch rewrites `red_channel_client_*(rcc, ...)` functions
> and calls into object methods.
>
> One aspect that could be discussed is the usage of the implicit
> `this`:
>
>     red_channel_client_push(rcc); // old
>     push(); // new, inside another RedChannelClient method
>
> my preference would be for avoiding this implicit `this`:
>
>     this->push();
>
> I think it makes the code easier to read, as you immediately know that
> `push` belongs (or is inherited) to the current class.
>
> I don't know how common this is in C++, but a Python motto is
> `Explicit is better than implicit`.
>
> Besides that, the patch looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

The use of "this" usually is pretty limited in C++ general styl,
to distinguish in case were is ambiguous or to retrieve just the
pointer to the object.
Other languages have much more explicit usage of this/self but for
other reasons (Python being a dynamic language, other because they
are not much object oriented).
I was discussion the distinguish and I found a "-Wshadow" warning in
gcc which could avoid this issue.

>
> commit 38cd152952968e37d0cc96e95e3e5e47a2f66c2f
> Author: Frediano Ziglio <fziglio at redhat.com>
> Date:   Sun Mar 1 19:38:38 2020 +0000
>
>   automake: Link with C++ linker
>
>   If automake sees no C++ files in the source it assumes have to
>   use C linker settings not linking C++ library.
>   This was not a problem as code did not use C++ libraries but next
>   patch will use pure virtual function call.
>   It could be provided but as later we will use RTTI use C++ library.
>
>   Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>
> This patch adds a `dummy.cpp` in the SOURCES files in order to trick
> the linker to use C++ linking. The linker is aware of this trick and
> doesn't complain about the missing file.
>
> The patch looks good to me.
>
> Acked-by: Kevin Pouget <kpouget at redhat.com>

yes, crazy trick, but it was on Automake manuals!

> 
> 
> 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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 