On Wed, Mar 07, 2018 at 11:28:41AM +0100, Christophe de Dinechin wrote:
>
>
> > On 6 Mar 2018, at 17:15, Christophe Fergeau <cferg...@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 <cferg...@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
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
