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

[Christophe Fergeau]

On Tue, Jul 03, 2018 at 04:59:12PM -0500, Jonathon Jongsma wrote:
> 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).

Yes, it can be refined a little bit (in particular, no commit logs), but
I've split that into https://gitlab.com/teuf/spice-common/commits/quic

My main comment would be regarding the addition of CHANNEL_ and
CHANNEL_ARG_, it seems to me it could be nicer if this became a
ENCODER_AND_CHANNEL_ and expanded to either Encoder *encoder or Encoder
*encoder, Channel *channl. I think this would make it look a bit more
like an actual arg (ie avoid (Encoder *encoder, CHANNEL_ARG_ int foo),
with CHANNEL_ARG_ being either empty or 'Channel *channel,'. We'd have
(ENCODER_AND_CHANNEL_ARG_, int foo);. Or even wrap that in a macro call
like ENCODER_FUNC(func_name)(int foo)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180704/0877b994/attachment.sig>