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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> > On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin at redhat.com>
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 47 ++++++++++++++++++++++++++-----------------
> > > 1 file changed, 28 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 69c27a3..1e19e43 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -181,19 +181,24 @@ write_all(int fd, const void *buf, const size_t len)
> > >     return written;
> > > }
> > > 
> > > -static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, unsigned c)
> > > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, uint8_t c)
> > > {
> > > -
> > > -    SpiceStreamFormatMessage msg;
> > > -    const size_t msgsize = sizeof(msg);
> > > -    const size_t hdrsize  = sizeof(msg.hdr);
> > > -    memset(&msg, 0, msgsize);
> > > -    msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > > -    msg.hdr.type = STREAM_TYPE_FORMAT;
> > > -    msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
> > > -    msg.msg.width = w;
> > > -    msg.msg.height = h;
> > > -    msg.msg.codec = c;
> > > +    const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> > > +    const size_t hdrsize  = sizeof(StreamDevHeader);
> > > +    SpiceStreamFormatMessage msg = {
> > > +        .hdr = {
> > > +            .protocol_version = STREAM_DEVICE_PROTOCOL,
> > > +            .padding = 0,       // Workaround GCC "not implemented" bug
> > > +            .type = STREAM_TYPE_FORMAT,
> > > +            .size = msgsize - hdrsize
> > > +        },
> > > +        .msg = {
> > > +            .width = w,
> > > +            .height = h,
> > > +            .codec = c,
> > > +            .padding1 = { }
> > > +        }
> > > +    };
> > >     syslog(LOG_DEBUG, "writing format\n");
> > >     std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > >     if (write_all(streamfd, &msg, msgsize) != msgsize) {
> > > @@ -204,14 +209,18 @@ static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, unsign
> > > 
> > > static int spice_stream_send_frame(int streamfd, const void *buf, const unsigned size)
> > > {
> > > -    SpiceStreamDataMessage msg;
> > > -    const size_t msgsize = sizeof(msg);
> > >     ssize_t n;
> > > +    const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> > > +    SpiceStreamDataMessage msg = {
> > > +        .hdr = {
> > > +            .protocol_version = STREAM_DEVICE_PROTOCOL,
> > > +            .padding = 0,       // Workaround GCC "not implemented" bug
> > > +            .type = STREAM_TYPE_DATA,
> > > +            .size = size  /* includes only the body? */
> > > +        },
> > > +        .msg = {}
> > > +    };
> > 
> > So, someone should find out if we can use the designated initializers,
> > I suppose it depends on the compilers on all platforms we care about
> > supporting them?
> > 
> > I wasn't able to find much useful information so far. Anyone knows in
> > which version of gcc it was introduced?
> 
> My experience is that clang has no issue with it in any version.
> 
> GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with the following short example:
> 
> struct Thing { int x; float y; };
> 
> int foo()
> {
>   Thing x = { x: 10, y:20 };
>   Thing y = { .x = 10, .y = 20 };
>   Thing z { 10, 20 };
>   Thing t { .x = 10, .y = 20 };
> }
> 
> It has, however, trouble with out-of-order initializations, with the same message in 4.8.5 as on Fedora 25 (6.4.1):
> 
> glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers not supported
>    Thing a { .y = 10, .x = 20 };
> 
> The “not implemented” message is a bit scary, but the fact that the code as written has been supported as far back as 4.8.5 makes me confident that we are not in some experimental section of the compiler.

I've found this on the matter:

https://gcc.gnu.org/ml/gcc/2014-09/msg00207.html

That doesn't give much confidence... Is this documented anywhere? I
mean I've only used Google and haven't found anything...

OTOH, if the extension is robust against random mistakes and typos, and
GCC and clang are the only compilers we care about, I think it should
be fine?

Lukas

> The alternatives are:
> - Not tagging fields at all
> - Tagging them “the old way”, i.e. field:value, but that has been deprecated since 2.5 and actually has the same bug as .field = value, so no benefit.
> 
> 
> > 
> > Lukas
> > 
> > > -    memset(&msg, 0, msgsize);
> > > -    msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > > -    msg.hdr.type = STREAM_TYPE_DATA;
> > > -    msg.hdr.size = size; /* includes only the body? */
> > >     std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > >     n = write_all(streamfd, &msg, msgsize);
> > >     syslog(LOG_DEBUG,
> > > @@ -379,7 +388,7 @@ do_capture(int streamfd, const char *streamport, FILE *f_log)
> > > 
> > >             if (frame.stream_start) {
> > >                 unsigned width, height;
> > > -                unsigned char codec;
> > > +                uint8_t codec;
> > > 
> > >                 width = frame.size.width;
> > >                 height = frame.size.height;
> 
>