>
> > 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.
> > - 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.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel