[Spice-devel] [PATCH 06/22] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe Fergeau]

On Wed, Mar 07, 2018 at 11:28:41AM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 6 Mar 2018, at 17:15, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> >>> On 28 Feb 2018, at 17:36, Christophe Fergeau <cfergeau at redhat.com> wrote:
> >>> 
> >>> My understanding is that the previous iteration was quite controversial,
> >>> I would just drop it from the series unless you get acks from everyone
> >>> involved this time.
> >> 
> >> It’s a bit difficult to drop that from the series, as it is a core element of the next steps if you look carefully.
> > 
> > I only looked at the code with the full series applied, but it really seems like both way would
> > be possible?
> > 
> > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > index fe8564f..2e1472a 100644
> > --- a/src/concrete-agent.cpp
> > +++ b/src/concrete-agent.cpp
> > @@ -140,7 +140,10 @@ public:
> >     }
> >     void write_message_body(Stream &stream, unsigned w, unsigned h, uint8_t c)
> >     {
> > -        StreamMsgFormat msg = { .width = w, .height = h, .codec = c, .padding1 = {} };
> > +        StreamMsgFormat msg;
> > +        msg.width = w;
> > +        msg.height = h;
> > +        msg.codec = c;
> >         stream.write_all("format", &msg, sizeof(msg));
> >     }
> > };
> > diff --git a/src/message.hpp b/src/message.hpp
> > index fd69033..674e122 100644
> > --- a/src/message.hpp
> > +++ b/src/message.hpp
> > @@ -21,13 +21,12 @@ class Message
> > public:
> >     template <typename ...PayloadArgs>
> >     Message(PayloadArgs... payload_args)
> > -        : hdr(StreamDevHeader {
> > -              .protocol_version = STREAM_DEVICE_PROTOCOL,
> > -              .padding = 0,     // Workaround GCC bug "sorry: not implemented"
> > -              .type = Type,
> > -              .size = (uint32_t) Info::size(payload_args...)
> > -          })
> > -    { }
> > +    {
> > +        hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > +        hdr.padding = 0;
> > +        hdr.type = Type;
> > +        hdr.size = (uint32_t) Info::size(payload_args...);
> > +    }
> >     void write_header(Stream &stream)
> >     {
> >         stream.write_all("header", &hdr, sizeof(hdr));
> > 
> > Not strongly advocating for that change to be made just now, I was just
> > a bit surprised by how you dismissed this ;)
> 
> I did not dismiss it, and I regret you interpreted it that way ;-)
> 
> The meaning I gave to “core element of the next steps” was that practically every patch touches that part in one way or another. So changing it and keeping it consistent essentially means redoing a good fraction of the whole series by hand. That’s a lot of churn.
> 
> I could do it if I saw value in doing the change. But I feel like I replied to Frediano’s objections on this topic, as well as to yours. I also believe that the code is significantly simpler, more type-safe and more readable the way I wrote it, for example because we get a warning or an error if we forget a field.
> 
> Does that address your objection?

Ok, you meant "this is going to cause rebase conflicts on several
patches", rather than "the next patches *require* 
initializating fields this way or they won't work", which is what I
thought you meant...

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/20180307/019054d5/attachment.sig>