Re: [PATCH spice-common 0/6] quic: Unify the 2 template files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2018-06-28 at 14:23 +0100, Frediano Ziglio wrote:
> QUIC is implemented using 2 C template files.
> One for single channel and one for multiple (RGB) channels.
> Unify the 2 templates to have a single source.
> 
> Frediano Ziglio (6):
>   quic: Call encode from golomb_coding
>   quic: Continue template unification
>   quic: Other template unification
>   quic: More template unification, unify macros
>   quic: Make the template identical
>   quic: Remove duplicate file
> 
>  common/Makefile.am        |   1 -
>  common/quic.c             |  31 +-
>  common/quic_family_tmpl.c |   6 +-
>  common/quic_rgb_tmpl.c    | 689 ------------------------------------
> --
>  common/quic_tmpl.c        | 403 ++++++++++++++--------
>  5 files changed, 264 insertions(+), 866 deletions(-)
>  delete mode 100644 common/quic_rgb_tmpl.c
> 


So, after reading through this series, I feel that the individual patch
splits were a bit arbitrary. Each patch contains a bunch of individual
sort-of-related changes. But I can't really see much justification for
where the splits were made, to be honest. (After writing this, teuf
told me he was already looking at these patches and had started
splitting them).

As for the end result, I suppose there is some benefit to unifying
these (and getting rid of the extra file). But to be honest, I'm a bit
ambivalent about it all since it adds some macro stuff that I don't
particularly like. On the other hand, I don't see us touching these
files regularly in the future, so it probably won't add much
maintenance burden.

Out of curiosity, why did you decide to refactor this? Does it enable
some other changes later? 

Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]