> On 8 Mar 2018, at 11:39, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>>
>>> On 7 Mar 2018, at 11:50, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>>
>>>>> On 6 Mar 2018, at 17:15, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>> wrote:
>>>>>>>
>>>>>>> My understanding is that the previous iteration was quite
>>>>>>> controversial,
>>>>>>> I would just drop it from the series unless you get acks from everyone
>>>>>>> involved this time.
>>>>>>
>>>>>> It’s a bit difficult to drop that from the series, as it is a core
>>>>>> element
>>>>>> of the next steps if you look carefully.
>>>>>
>>>>> I only looked at the code with the full series applied, but it really
>>>>> seems
>>>>> like both way would
>>>>> be possible?
>>>>>
>>>>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>>>>> index fe8564f..2e1472a 100644
>>>>> --- a/src/concrete-agent.cpp
>>>>> +++ b/src/concrete-agent.cpp
>>>>> @@ -140,7 +140,10 @@ public:
>>>>> }
>>>>> void write_message_body(Stream &stream, unsigned w, unsigned h,
>>>>> uint8_t
>>>>> c)
>>>>> {
>>>>> - StreamMsgFormat msg = { .width = w, .height = h, .codec = c,
>>>>> .padding1 = {} };
>>>>> + StreamMsgFormat msg;
>>>>> + msg.width = w;
>>>>> + msg.height = h;
>>>>> + msg.codec = c;
>>>>> stream.write_all("format", &msg, sizeof(msg));
>>>>> }
>>>>> };
>>>>> diff --git a/src/message.hpp b/src/message.hpp
>>>>> index fd69033..674e122 100644
>>>>> --- a/src/message.hpp
>>>>> +++ b/src/message.hpp
>>>>> @@ -21,13 +21,12 @@ class Message
>>>>> public:
>>>>> template <typename ...PayloadArgs>
>>>>> Message(PayloadArgs... payload_args)
>>>>> - : hdr(StreamDevHeader {
>>>>> - .protocol_version = STREAM_DEVICE_PROTOCOL,
>>>>> - .padding = 0, // Workaround GCC bug "sorry: not
>>>>> implemented"
>>>>> - .type = Type,
>>>>> - .size = (uint32_t) Info::size(payload_args...)
>>>>> - })
>>>>> - { }
>>>>> + {
>>>>> + hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>>>>> + hdr.padding = 0;
>>>>> + hdr.type = Type;
>>>>> + hdr.size = (uint32_t) Info::size(payload_args...);
>>>>> + }
>>>>> void write_header(Stream &stream)
>>>>> {
>>>>> stream.write_all("header", &hdr, sizeof(hdr));
>>>>>
>>>>> Not strongly advocating for that change to be made just now, I was just
>>>>> a bit surprised by how you dismissed this ;)
>>>>
>>>> I did not dismiss it, and I regret you interpreted it that way ;-)
>>>>
>>>> The meaning I gave to “core element of the next steps” was that
>>>> practically
>>>> every patch touches that part in one way or another. So changing it and
>>>> keeping it consistent essentially means redoing a good fraction of the
>>>> whole
>>>> series by hand. That’s a lot of churn.
>>>>
>>>> I could do it if I saw value in doing the change. But I feel like I
>>>> replied
>>>> to Frediano’s objections on this topic, as well as to yours. I also
>>>> believe
>>>> that the code is significantly simpler, more type-safe and more readable
>>>> the
>>>> way I wrote it, for example because we get a warning or an error if we
>>>> forget a field.
>>>>
>>>
>>> I confirm, the problem of the paddings and security was discussed
>>> and agreed is not a problem.
>>
>> Thanks.
>>
>>>
>>>> Does that address your objection?
>>>>
>>>>>
>>>>> Christophe
>>>>
>>>
>>> There are however still some issues:
>>> - the syntax is using C++20 while we state we use C++11 syntax, this
>>> is basically using C compatibility extensions. I just tried and for
>>> instance this code is not accepted on Visual C++ 2015 (not an issue
>>> at the moment);
>>
>> No, but it is annoying. Will make that obvious in the commit log.
>>
>
> I don't think that a comment on the log will make Visual C++
> compile that code. Stating C++11 was the reason of this, not use too
> advance syntax that could have problems.
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 misleading, the protocol is defined that way not for
>>> inconsistency but being a C file is subject to C limits and
>>> you cannot portably specify a binary size for enumerations.
>>
>> I understand the rationale, and I can add it. But the comment is not
>> misleading, it was a response to a question you had about the reason for the
>> type change (the reason being that I had a warning without it).
>>
>> Changed in working branch to:
>>
>> Also note that designated initializers is a C99 or C++20 feature.
>> It is presently accepted by both gcc and clang, but not VS2015.
>> Also note that gcc is picky relative to clang, but closer to the
>> current working draft for C++20, in that it requires fields to be
>> initialized in declaration order and to all be initialized, and that
>> you don't get a clear error message but an "unsupported" or
>> "unimplementd" message if you do not follow that rule.
>>
>> Writing this patch also highlighted an inconsistency between the type
>> of a 'codec' local variable and the type used in the actual data
>> type (which must obey C rules, since it's in the protocol).
>> In order to avoid a warning, this patch changes the type of the
>> variable to uint8_t to match that in the structure declaration.
>>
>>
>>>
>>> As I said previously if the problem is the time that take to
>>> do the change to the patch I can do it.
>>
>> It will not really help, since that series already received a fair number of
>> changes, and so you patch would add on top of the pile.
>>
>
> Not proposing to add patches, to change them.
I understand. What I’m saying is that my working branch is already quite a bit ahead of what you have to work with. You may start from my private branch https://github.com/c3d/spice-streaming-agent/commits/c%2B%2B-refactoring.
I’d appreciate suggestions as where to put the cut point to stick with what was reviewed already. Some stuff in there is new, e.g. adding export-dynamic right away. The exception handling API shows no sign that either sign manages to convince the other.
At this point, I am not sending again because I’m blocked by this discussion, and that exception handling patch is right in the middle.
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel