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

[Frediano Ziglio]

> 
> > On 7 Mar 2018, at 11:50, Frediano Ziglio <fziglio at redhat.com> 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.
> >> 
> > 
> > I confirm, the problem of the paddings and security was discussed
> > and agreed is not a problem.
> 
> Thanks.
> 
> > 
> >> Does that address your objection?
> >> 
> >>> 
> >>> Christophe
> >> 
> > 
> > There are however still some issues:
> > - the syntax is using C++20 while we state we use C++11 syntax, this
> >  is basically using C compatibility extensions. I just tried and for
> >  instance this code is not accepted on Visual C++ 2015 (not an issue
> >  at the moment);
> 
> No, but it is annoying. Will make that obvious in the commit log.
> 

I don't think that a comment on the log will make Visual C++
compile that code. Stating C++11 was the reason of this, not use too
advance syntax that could have problems.

> > - the "Also note that GCC is a bit picky and will generate strange
> >  "unsupported" or "not implemented" errors if the fields are not all
> >  initialized in exact declaration order." is wrong, this is not a
> >  GCC errors, just that GCC is respecting C++. Fields are required
> >  to be initialized in the correct order in C++ (this differs from
> >  C99);
> 
> It’s picky relative to clang. But I agree that rephrasing the comment would
> improve it.
> 
> (I don’t want to refer to C++20, since this is WIP and that specific aspect
> may still change)
> 
> 
> > - the "Writing this patch also highlighted an inconsistency between the
> >  type of a 'codec' local variable and the type used in the actual data
> >  type." is misleading, the protocol is defined that way not for
> >  inconsistency but being a C file is subject to C limits and
> >  you cannot portably specify a binary size for enumerations.
> 
> I understand the rationale, and I can add it. But the comment is not
> misleading, it was a response to a question you had about the reason for the
> type change (the reason being that I had a warning without it).
> 
> Changed in working branch to:
> 
> Also note that designated initializers is a C99 or C++20 feature.
> It is presently accepted by both gcc and clang, but not VS2015.
> Also note that gcc is picky relative to clang, but closer to the
> current working draft for C++20, in that it requires fields to be
> initialized in declaration order and to all be initialized, and that
> you don't get a clear error message but an "unsupported" or
> "unimplementd" message if you do not follow that rule.
> 
> Writing this patch also highlighted an inconsistency between the type
> of a 'codec' local variable and the type used in the actual data
> type (which must obey C rules, since it's in the protocol).
> In order to avoid a warning, this patch changes the type of the
> variable to uint8_t to match that in the structure declaration.
> 
> 
> > 
> > As I said previously if the problem is the time that take to
> > do the change to the patch I can do it.
> 
> It will not really help, since that series already received a fair number of
> changes, and so you patch would add on top of the pile.
> 

Not proposing to add patches, to change them.

Frediano