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

Thu Mar 8 10:32:21 UTC 2018

> On 7 Mar 2018, at 11:50, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> On 6 Mar 2018, at 17:15, Christophe Fergeau <cfergeau at redhat.com> wrote:
>>> 
>>> On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
>>>>> On 28 Feb 2018, at 17:36, Christophe Fergeau <cfergeau at redhat.com> wrote:
>>>>> 
>>>>> My understanding is that the previous iteration was quite controversial,
>>>>> I would just drop it from the series unless you get acks from everyone
>>>>> involved this time.
>>>> 
>>>> It’s a bit difficult to drop that from the series, as it is a core element
>>>> of the next steps if you look carefully.
>>> 
>>> I only looked at the code with the full series applied, but it really seems
>>> like both way would
>>> be possible?
>>> 
>>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>>> index fe8564f..2e1472a 100644
>>> --- a/src/concrete-agent.cpp
>>> +++ b/src/concrete-agent.cpp
>>> @@ -140,7 +140,10 @@ public:
>>>    }
>>>    void write_message_body(Stream &stream, unsigned w, unsigned h, uint8_t
>>>    c)
>>>    {
>>> -        StreamMsgFormat msg = { .width = w, .height = h, .codec = c,
>>> .padding1 = {} };
>>> +        StreamMsgFormat msg;
>>> +        msg.width = w;
>>> +        msg.height = h;
>>> +        msg.codec = c;
>>>        stream.write_all("format", &msg, sizeof(msg));
>>>    }
>>> };
>>> diff --git a/src/message.hpp b/src/message.hpp
>>> index fd69033..674e122 100644
>>> --- a/src/message.hpp
>>> +++ b/src/message.hpp
>>> @@ -21,13 +21,12 @@ class Message
>>> public:
>>>    template <typename ...PayloadArgs>
>>>    Message(PayloadArgs... payload_args)
>>> -        : hdr(StreamDevHeader {
>>> -              .protocol_version = STREAM_DEVICE_PROTOCOL,
>>> -              .padding = 0,     // Workaround GCC bug "sorry: not
>>> implemented"
>>> -              .type = Type,
>>> -              .size = (uint32_t) Info::size(payload_args...)
>>> -          })
>>> -    { }
>>> +    {
>>> +        hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>>> +        hdr.padding = 0;
>>> +        hdr.type = Type;
>>> +        hdr.size = (uint32_t) Info::size(payload_args...);
>>> +    }
>>>    void write_header(Stream &stream)
>>>    {
>>>        stream.write_all("header", &hdr, sizeof(hdr));
>>> 
>>> Not strongly advocating for that change to be made just now, I was just
>>> a bit surprised by how you dismissed this ;)
>> 
>> I did not dismiss it, and I regret you interpreted it that way ;-)
>> 
>> The meaning I gave to “core element of the next steps” was that practically
>> every patch touches that part in one way or another. So changing it and
>> keeping it consistent essentially means redoing a good fraction of the whole
>> series by hand. That’s a lot of churn.
>> 
>> I could do it if I saw value in doing the change. But I feel like I replied
>> to Frediano’s objections on this topic, as well as to yours. I also believe
>> that the code is significantly simpler, more type-safe and more readable the
>> way I wrote it, for example because we get a warning or an error if we
>> forget a field.
>> 
> 
> I confirm, the problem of the paddings and security was discussed
> and agreed is not a problem.

Thanks.

> 
>> Does that address your objection?
>> 
>>> 
>>> Christophe
>> 
> 
> There are however still some issues:
> - the syntax is using C++20 while we state we use C++11 syntax, this
>  is basically using C compatibility extensions. I just tried and for
>  instance this code is not accepted on Visual C++ 2015 (not an issue
>  at the moment);

No, but it is annoying. Will make that obvious in the commit log.

> - the "Also note that GCC is a bit picky and will generate strange
>  "unsupported" or "not implemented" errors if the fields are not all
>  initialized in exact declaration order." is wrong, this is not a
>  GCC errors, just that GCC is respecting C++. Fields are required
>  to be initialized in the correct order in C++ (this differs from
>  C99);

It’s picky relative to clang. But I agree that rephrasing the comment would improve it.

(I don’t want to refer to C++20, since this is WIP and that specific aspect may still change)

> - the "Writing this patch also highlighted an inconsistency between the
>  type of a 'codec' local variable and the type used in the actual data
>  type." is misleading, the protocol is defined that way not for
>  inconsistency but being a C file is subject to C limits and
>  you cannot portably specify a binary size for enumerations.

I understand the rationale, and I can add it. But the comment is not misleading, it was a response to a question you had about the reason for the type change (the reason being that I had a warning without it).

Changed in working branch to:

Also note that designated initializers is a C99 or C++20 feature.
It is presently accepted by both gcc and clang, but not VS2015.
Also note that gcc is picky relative to clang, but closer to the
current working draft for C++20, in that it requires fields to be
initialized in declaration order and to all be initialized, and that
you don't get a clear error message but an "unsupported" or
"unimplementd" message if you do not follow that rule.

Writing this patch also highlighted an inconsistency between the type
of a 'codec' local variable and the type used in the actual data
type (which must obey C rules, since it's in the protocol).
In order to avoid a warning, this patch changes the type of the
variable to uint8_t to match that in the structure declaration.

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

It will not really help, since that series already received a fair number of changes, and so you patch would add on top of the pile.

> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel