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

[Christophe de Dinechin]

> On 23 Feb 2018, at 12:08, Christophe Fergeau  wrote:
> 
> On Fri, Feb 23, 2018 at 12:01:59PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 23 Feb 2018, at 10:53, Christophe Fergeau  wrote:
>>> 
>>> Given the lengthy debate over what is mostly a small cosmetic patch, I
>>> suggest that we postpone this one for now and drop it from the series.
>> 
>> memset in C++ code is not just a style issue, it’s dangerous. It completely 
>> wipes out C++ type guarantees. For example, if someone inits a field with
>> 
>>  int x = 1;
>> 
>> Then all constructors will guarantee that x == -1, but a memset after
>> object creation wipes out that guarantee. Same thing if we make of of
>> the objects being memset-initialized contain some C++ object with a
>> vtable. And so on. All these problems do not exist with C++
>> zero-initialization.
> 
> Is this an actual problem with the 2 structs which are being discussed
> here? In other word, is this patch currently fixing a bug? I don't think
> it does, so it can safely be postponed for a later time when people get
> to an agreement on it, or when we have less patches pending, ...
> 
>> Which is also significantly shorter to write.
> 
> I did not mention it the first time, but this patch is added more lines
> that it removes. So I'll beg to disagree with the "shorter" part ;)

Petty, because we were specifically talking about zero-init, i.e.:

foo x = {};

is shorter than
foo x;
memset(, 0, sizeof(x));


But since you brought a new point, you counted lines. If count bytes, the first 
section of my patch is 381 bytes, it was 473 bytes before, so yes, “shorter” in 
bytes :-) And frankly, I wish I did not have to spend time countering this kind 
of argument!


Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

On Fri, Feb 23, 2018 at 12:01:59PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 23 Feb 2018, at 10:53, Christophe Fergeau  wrote:
> > 
> > Given the lengthy debate over what is mostly a small cosmetic patch, I
> > suggest that we postpone this one for now and drop it from the series.
> 
> memset in C++ code is not just a style issue, it’s dangerous. It completely 
> wipes out C++ type guarantees. For example, if someone inits a field with
> 
>   int x = 1;
> 
> Then all constructors will guarantee that x == -1, but a memset after
> object creation wipes out that guarantee. Same thing if we make of of
> the objects being memset-initialized contain some C++ object with a
> vtable. And so on. All these problems do not exist with C++
> zero-initialization.

Is this an actual problem with the 2 structs which are being discussed
here? In other word, is this patch currently fixing a bug? I don't think
it does, so it can safely be postponed for a later time when people get
to an agreement on it, or when we have less patches pending, ...

> Which is also significantly shorter to write.

I did not mention it the first time, but this patch is added more lines
that it removes. So I'll beg to disagree with the "shorter" part ;)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On 23 Feb 2018, at 10:53, Christophe Fergeau  wrote:
> 
> Given the lengthy debate over what is mostly a small cosmetic patch, I
> suggest that we postpone this one for now and drop it from the series.

memset in C++ code is not just a style issue, it’s dangerous. It completely 
wipes out C++ type guarantees. For example, if someone inits a field with

int x = 1;

Then all constructors will guarantee that x == -1, but a memset after object 
creation wipes out that guarantee. Same thing if we make of of the objects 
being memset-initialized contain some C++ object with a vtable. And so on. All 
these problems do not exist with C++ zero-initialization. Which is also 
significantly shorter to write.

Frankly, there should be no “lengthy debate” about this.


Regards,
Christophe

> 
> Christophe
> 
> On Fri, Feb 16, 2018 at 05:15:37PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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(, 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 stream_guard(stream_mtx);
>> if (write_all(streamfd, , 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 = {}
>> +};
>> 
>> -memset(, 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 stream_guard(stream_mtx);
>> n = write_all(streamfd, , 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;
>> -- 
>> 2.13.5 (Apple Git-94)
>> 
>> ___
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On 22 Feb 2018, at 19:03, Frediano Ziglio  wrote:
> 
>> 
>> [Indeed, I had not sent this reply, took time to check standard and
>> compilers]
>> 
>>> On 20 Feb 2018, at 12:31, Frediano Ziglio  wrote:
>>> 
> On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
>>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 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(, 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 stream_guard(stream_mtx);
  if (write_all(streamfd, , 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:

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

[Christophe Fergeau]

Given the lengthy debate over what is mostly a small cosmetic patch, I
suggest that we postpone this one for now and drop it from the series.

Christophe

On Fri, Feb 16, 2018 at 05:15:37PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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(, 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 stream_guard(stream_mtx);
>  if (write_all(streamfd, , 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 = {}
> +};
>  
> -memset(, 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 stream_guard(stream_mtx);
>  n = write_all(streamfd, , 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;
> -- 
> 2.13.5 (Apple Git-94)
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

[Indeed, I had not sent this reply, took time to check standard and compilers]

> On 20 Feb 2018, at 12:31, Frediano Ziglio  wrote:
> 
>>> On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
>>> 
>>> On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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(, 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 stream_guard(stream_mtx);
>>   if (write_all(streamfd, , 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…
>> 
>> I think it’s a low priority thing because the “fix” in source code is so easy
>> (reorder the fields), and the fix complicated. According to the message you
>> referenced, 

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

[Frediano Ziglio]

> > On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
> > 
> > On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> >>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> >>> 
> >>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>  From: Christophe de Dinechin 
>  
>  Signed-off-by: Christophe de Dinechin 
>  ---
>  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(, 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 stream_guard(stream_mtx);
> if (write_all(streamfd, , 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…
> 
> I think it’s a low priority thing because the “fix” in source code is so easy
> (reorder the fields), and the fix complicated. According to the message you
> referenced, presently, GCC just checks that the annotations match the names,
> but otherwise ignores them and proceeds like for a POD init. If it were to
> be 

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

[Christophe de Dinechin]

> On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
>>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 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(, 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 stream_guard(stream_mtx);
if (write_all(streamfd, , 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…

I think it’s a low priority thing because the “fix” in source code is so easy 
(reorder the fields), and the fix complicated. According to the message you 
referenced, presently, GCC just checks that the annotations match the names, 
but otherwise ignores them and proceeds like for a POD init. If it were to be 
implemented, it would have to support a lot of corner cases mentioned in the 
message, so this makes the fix non-trivial.

> 
> OTOH, if the extension is robust against random mistakes and typos, and
> GCC and clang are the only compilers we 

Re: [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ý  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > 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(, 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 stream_guard(stream_mtx);
> > > if (write_all(streamfd, , 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(, 0, msgsize);
> > > -msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > > -msg.hdr.type = STREAM_TYPE_DATA;
> > > -msg.hdr.size = size; /* includes only the body? 

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

[Christophe de Dinechin]

> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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(, 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 stream_guard(stream_mtx);
>> if (write_all(streamfd, , 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.

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(, 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 stream_guard(stream_mtx);
>> n = write_all(streamfd, , 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;

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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(, 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 stream_guard(stream_mtx);
>  if (write_all(streamfd, , 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?

Lukas

> -memset(, 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 stream_guard(stream_mtx);
>  n = write_all(streamfd, , 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;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel