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

[Frediano Ziglio]

> 
> > On 8 Mar 2018, at 12:42, Christophe Fergeau  wrote:
> > 
> > On Thu, Mar 08, 2018 at 05:39:48AM -0500, Frediano Ziglio wrote:
>  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.
> > 
> > For what it's worth, I'd be in favour of *not* using things newer than
> > what is in c++11 (or maybe c++14), this would give some first guideline
> > as to what's ok to use, and what should be avoided.
> 
> I agree.
> 
> However, we presently use gnu++11, so this is C++11 with GNU extensions.
> As an extension, designated initializers have been in GCC since at least 4.7.
> 
> So do you think we should avoid GCC extensions and switch to “strict” mode?
> 
> 
> Christophe

No, you don't agree, C++11 is not gnu++11.
IMHO your argument is OT. There's a difference in including external code
that uses extensions or use extensions in a portable way if strictly needed
or not breaking portability.

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

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

[Christophe de Dinechin]

> On 8 Mar 2018, at 12:42, Christophe Fergeau  wrote:
> 
> On Thu, Mar 08, 2018 at 05:39:48AM -0500, Frediano Ziglio wrote:
 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.
> 
> For what it's worth, I'd be in favour of *not* using things newer than
> what is in c++11 (or maybe c++14), this would give some first guideline
> as to what's ok to use, and what should be avoided.

I agree.

However, we presently use gnu++11, so this is C++11 with GNU extensions.
As an extension, designated initializers have been in GCC since at least 4.7.

So do you think we should avoid GCC extensions and switch to “strict” mode?


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

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

[Frediano Ziglio]

> 
> On Thu, Mar 08, 2018 at 05:39:48AM -0500, Frediano Ziglio wrote:
> > > > 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.
> 
> For what it's worth, I'd be in favour of *not* using things newer than
> what is in c++11 (or maybe c++14), this would give some first guideline
> as to what's ok to use, and what should be avoided.
> 
> Christophe
> 

I agree, when I read language version XX style I read as both:
- compatibility: we don't use syntaxes newer than XX;
- requirement: we don't support compilers not compatible with XX.
I though this was a standard interpretation and we quickly agree
on C++11 so I though nobody objected.

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

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

[Christophe Fergeau]

On Thu, Mar 08, 2018 at 05:39:48AM -0500, Frediano Ziglio wrote:
> > > 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.

For what it's worth, I'd be in favour of *not* using things newer than
what is in c++11 (or maybe c++14), this would give some first guideline
as to what's ok to use, and what should be avoided.

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 06/22] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe de Dinechin]

> On 8 Mar 2018, at 11:39, Frediano Ziglio  wrote:
> 
>>> 
>>> On 7 Mar 2018, at 11:50, Frediano Ziglio  wrote:
>>> 
> On 6 Mar 2018, at 17:15, Christophe Fergeau  wrote:
> 
> On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
>>> On 28 Feb 2018, at 17:36, Christophe Fergeau 
>>> 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 , 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", , 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 
>   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.write_all("header", , 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.

It won’t. But you stated “not an issue at the moment”.

What do you want then? A change to get rid of designated initializers, or a 
commit log that states we did that knowingly?

> 
>>> - 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 

Re: [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  wrote:
> > 
> >>> On 6 Mar 2018, at 17:15, Christophe Fergeau  wrote:
> >>> 
> >>> On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> > On 28 Feb 2018, at 17:36, Christophe Fergeau 
> > 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 , 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", , 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 
> >>>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.write_all("header", , 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
> 

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

[Christophe de Dinechin]

> On 7 Mar 2018, at 11:50, Frediano Ziglio  wrote:
> 
>>> On 6 Mar 2018, at 17:15, Christophe Fergeau  wrote:
>>> 
>>> On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> On 28 Feb 2018, at 17:36, Christophe Fergeau  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 , 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", , 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 
>>>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.write_all("header", , 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.

> - 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 

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

[Frediano Ziglio]

> > On 6 Mar 2018, at 17:15, Christophe Fergeau  wrote:
> > 
> > On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> >>> On 28 Feb 2018, at 17:36, Christophe Fergeau  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 , 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", , 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 
> > 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.write_all("header", , 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.

> 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);
- 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);
- 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.

As I said previously if the problem is the time that take to
do the change to the patch I can do it.

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

Re: [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  wrote:
> > 
> > On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> >>> On 28 Feb 2018, at 17:36, Christophe Fergeau  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 , 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", , 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 
> > 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.write_all("header", , 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

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

[Christophe de Dinechin]

> On 6 Mar 2018, at 17:15, Christophe Fergeau  wrote:
> 
> On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
>>> On 28 Feb 2018, at 17:36, Christophe Fergeau  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 , 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", , 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 
> 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.write_all("header", , 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?

> 
> Christophe

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

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

[Christophe Fergeau]

On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> > On 28 Feb 2018, at 17:36, Christophe Fergeau  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 , 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", , 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 
 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.write_all("header", , sizeof(hdr));

Not strongly advocating for that change to be made just now, I was just
a bit surprised by how you dismissed this ;)

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 06/22] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe de Dinechin]

> On 1 Mar 2018, at 10:51, Lukáš Hrázký  wrote:
> 
> On Wed, 2018-02-28 at 17:36 +0100, Christophe Fergeau 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.
> 
> I've only commented on compiler support, it seems it's fine, so no
> problem for me. I'll leave the ack to Frediano, he understands better
> the padding issues.
> 
>> Christophe
>> 
>> On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote:
>>> From: Christophe de Dinechin 
>>> 
>>> C++-style aggregate initialization with explicit field names is more
>>> readable and requires less repeated typing.
>>> 
>>> Like zero initialization, aggregate initialization preserves C++ type
>>> safety.  However, unlike zero initialization, it does not guarantee
>>> that padding is properly initialized. This is not a real problem in
>>> our code where all padding is explicit, but it it worth keeping in
> 
> "it is”.

Fixed

> 
>>> mind.
>>> 
>>> 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. It is useful, however, to get
>>> an error if a field is added and not initialized.
>>> 
>>> Writing this patch also highlighted an inconsistency between the type
>>> of a 'codec' local variable and the type used in the actual data
>>> type. Changed the type of the variable to uint8_t to match that in the
>>> structure declaration.
>>> 
>>> 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 acae939..7b56458 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -221,19 +221,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) {
>>> @@ -244,14 +249,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? */
> 
> The question in the comment? :)

It was in the original. Not sure why.

> 
>>> +},
>>> +.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,
>>> @@ -424,7 +433,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 = 

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

[Christophe de Dinechin]

> On 28 Feb 2018, at 17:36, Christophe Fergeau  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.


> 
> Christophe
> 
> On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> C++-style aggregate initialization with explicit field names is more
>> readable and requires less repeated typing.
>> 
>> Like zero initialization, aggregate initialization preserves C++ type
>> safety.  However, unlike zero initialization, it does not guarantee
>> that padding is properly initialized. This is not a real problem in
>> our code where all padding is explicit, but it it worth keeping in
>> mind.
>> 
>> 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. It is useful, however, to get
>> an error if a field is added and not initialized.
>> 
>> Writing this patch also highlighted an inconsistency between the type
>> of a 'codec' local variable and the type used in the actual data
>> type. Changed the type of the variable to uint8_t to match that in the
>> structure declaration.
>> 
>> 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 acae939..7b56458 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -221,19 +221,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) {
>> @@ -244,14 +249,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,
>> @@ -424,7 +433,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
> 

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

[Lukáš Hrázký]

On Wed, 2018-02-28 at 17:36 +0100, Christophe Fergeau 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.

I've only commented on compiler support, it seems it's fine, so no
problem for me. I'll leave the ack to Frediano, he understands better
the padding issues.

> Christophe
> 
> On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin 
> > 
> > C++-style aggregate initialization with explicit field names is more
> > readable and requires less repeated typing.
> > 
> > Like zero initialization, aggregate initialization preserves C++ type
> > safety.  However, unlike zero initialization, it does not guarantee
> > that padding is properly initialized. This is not a real problem in
> > our code where all padding is explicit, but it it worth keeping in

"it is".

> > mind.
> > 
> > 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. It is useful, however, to get
> > an error if a field is added and not initialized.
> > 
> > Writing this patch also highlighted an inconsistency between the type
> > of a 'codec' local variable and the type used in the actual data
> > type. Changed the type of the variable to uint8_t to match that in the
> > structure declaration.
> > 
> > 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 acae939..7b56458 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -221,19 +221,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) {
> > @@ -244,14 +249,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? */

The question in the comment? :)

> > +},
> > +.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,
> > @@ -424,7 +433,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 

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

[Christophe Fergeau]

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.

Christophe

On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> C++-style aggregate initialization with explicit field names is more
> readable and requires less repeated typing.
> 
> Like zero initialization, aggregate initialization preserves C++ type
> safety.  However, unlike zero initialization, it does not guarantee
> that padding is properly initialized. This is not a real problem in
> our code where all padding is explicit, but it it worth keeping in
> mind.
> 
> 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. It is useful, however, to get
> an error if a field is added and not initialized.
> 
> Writing this patch also highlighted an inconsistency between the type
> of a 'codec' local variable and the type used in the actual data
> type. Changed the type of the variable to uint8_t to match that in the
> structure declaration.
> 
> 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 acae939..7b56458 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -221,19 +221,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) {
> @@ -244,14 +249,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,
> @@ -424,7 +433,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