array and enum attributes

Pekka Paalanen ppaalanen at gmail.com
Fri Nov 25 12:59:50 UTC 2016 

On Fri, 29 Jul 2016 22:51:28 +0200
"Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:

> Hi!
> 
> On Fri, Jul 29, 2016 at 10:52 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Fri, 22 Jul 2016 21:39:20 +0200
> > "Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:
> >  
> >> HI,
> >>
> >> On Thu, Jun 2, 2016 at 10:30 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> >> > On Thu, 2 Jun 2016 15:39:47 +0800
> >> > Jonas Ådahl <jadahl at gmail.com> wrote:
> >> >  
> >> >> On Thu, Jun 02, 2016 at 10:24:19AM +0300, Pekka Paalanen wrote:  
> >> >> > On Wed, 1 Jun 2016 13:16:31 -0500
> >> >> > Yong Bakos <junk at humanoriented.com> wrote:
> >> >> >  
> >> >> > > On May 30, 2016, at 3:54 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> >> >> > > >
> >> >> > > > On Sat, 28 May 2016 08:39:59 -0500
> >> >> > > > Yong Bakos <junk at humanoriented.com> wrote:
> >> >> > > >  
> >> >> > > >> Hi Mike,
> >> >> > > >> Regarding the combination of type="array" enum="foo"...
> >> >> > > >>  
> >> >> > > >>> On May 27, 2016, at 12:30 PM, Mike Blumenkrantz <michael.blumenkrantz at gmail.com> wrote:
> >> >> > > >>>
> >> >> > > >>> I've inlined some replies below.
> >> >> > > >>>
> >> >> > > >>> On Fri, May 27, 2016 at 1:13 PM Yong Bakos <junk at humanoriented.com <mailto:junk at humanoriented.com>> wrote:
> >> >> > > >>> On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <zmike at osg.samsung.com <mailto:zmike at osg.samsung.com>> wrote:  
> >> >> > > >>>>
> >> >> > > >>>> this adds a method for compositors to change various draw attributes
> >> >> > > >>>> for a surface
> >> >> > > >>>>
> >> >> > > >>>> Signed-off-by: Mike Blumenkrantz <zmike at osg.samsung.com <mailto:zmike at osg.samsung.com>>
> >> >> > > >>>> Signed-off-by: Jonas Ådahl <jadahl at gmail.com <mailto:jadahl at gmail.com>>  
> >> >> > > >>>
> >> >> > > >>> Hi Mike & Jonas,
> >> >> > > >>> A question about communicating default state, and some
> >> >> > > >>> minor nits you can certainly ignore, inline below.
> >> >> > > >>>
> >> >> > > >>>  
> >> >> > > >>>> ---
> >> >> > > >>>> unstable/xdg-shell/xdg-shell-unstable-v6.xml | 69 ++++++++++++++++++++++++++++
> >> >> > > >>>> 1 file changed, 69 insertions(+)
> >> >> > > >>>>
> >> >> > > >>>> diff --git a/unstable/xdg-shell/xdg-shell-unstable-v6.xml b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> >> >> > > >>>> index dfd7e84..0fa76d4 100644
> >> >> > > >>>> --- a/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> >> >> > > >>>> +++ b/unstable/xdg-shell/xdg-shell-unstable-v6.xml  
> >> >> > > >  
> >> >> > > >>>> +
> >> >> > > >>>> +        Calling this after an xdg_toplevel's first commit will raise a client error.
> >> >> > > >>>> +      </description>
> >> >> > > >>>> +      <arg name="states" type="array" enum="draw_state"/>  
> >> >> > > >>>
> >> >> > > >>> Just a sanity check, since I haven't seen it in a protocol spec yet. Does scanner handle
> >> >> > > >>> this combination of array and enum correctly?
> >> >> > > >>>
> >> >> > > >>> Good catch. This also affects the event above it.  
> >> >> > > >>
> >> >> > > >> As we discussed via IRC (27 May), the scanner will choke on this. While we talked about
> >> >> > > >> making a change to the scanner to allow this, perhaps such a change doesn't make sense.
> >> >> > > >>
> >> >> > > >> Given a type="array", scanner will generate a parameter of type wl_array.
> >> >> > > >>
> >> >> > > >> Perhaps the short story here is to just remove the enum from this arg, and the similar
> >> >> > > >> arg in the configure_draw_states event above. What do you think?
> >> >> > > >>
> >> >> > > >> (I wonder if it's the DTD that can change, so the scanner's validation step
> >> >> > > >> will catch the unsupported combination of type="array" enum="foo". My gut tells me that
> >> >> > > >> DTDs don't support this logic, but I'll dig into this.)  
> >> >> > > >
> >> >> > > > Hi,
> >> >> > > >
> >> >> > > > here is some background.
> >> >> > > >
> >> >> > > > A type="array" argument is really just a binary blob of data. The XML
> >> >> > > > description, human documentation aside, does not specify anything about
> >> >> > > > the blob contents. Therefore adding an XML attribute pointing to an
> >> >> > > > enum definition is half-useless. Generators could use it for creating
> >> >> > > > automatic links in documentation, but it cannot be used for code
> >> >> > > > generation, because you don't know the types contained in the blob.
> >> >> > > >
> >> >> > > > We also do not want to add blob content type definitions to the XML
> >> >> > > > language, because you might want to have everything C is able to
> >> >> > > > express, including nested structs. There is also no requirement that
> >> >> > > > the "array" is really an array - every "element" could be a different
> >> >> > > > thing. It could be bitstream and whatnot. Only the use of
> >> >> > > > wl_array_for_each() implies it is an array of similar elements,
> >> >> > > > wl_array_add() does not.  
> >>
> >> Then that's not an 'array' than but more like a struct.
> >> Therefore the structure definition should be in the XML.
> >> Leaving it undocumented is a terrible option.  
> >
> > Hi Nils,
> >
> > yes, things should definitely be documented. I mean human-readable
> > documentation, not necessarily machine-parseable description.
> >  
> >> >> > > > The big point in adding enum annotations was that language bindings
> >> >> > > > generators (other than wayland-scanner) could use the annotation for
> >> >> > > > code generation. I don't think it is possible with the array type.  
> >>
> >> Of course it is. If we properly define its contents, language bindings
> >> could put the
> >> proper structure into the argument list of the particular requests/events.  
> >
> > I would hate to have to define the XML language for describing
> > more complicated types than the three POD types we already have for
> > message arguments.  
> 
> I acually don't image it to be too complicated.
> To declare a structure, I'd imagine something like:
> 
> <struct name="foo">
>   <entry name="bar" type="string"/>
>   <entry name="baz" type="uint" enum="wl_shm.format"/>
>   <entry name="yada" type="array" array="other"/>
> </struct>
> 
> and then later:
> 
> <arg name="stuff" type="array" array="foo"/>
> 
> This would denote an array of structures of type "foo".
> Having an array of length 1 would be possible of course.

Hi,

yes, that how it starts. You forgot nested structs. What about
alignment? Packing? It gets out of hand fast, and you end up copying
the whole C spec and translating it all in a scanner. After all, the
underlying assumption is that you can describe (a subset of) C struct
definition in XML and be able to automatically cast a pointer pointing
into a wl_array to a struct pointer and expect it to work.

That's a lot of new XML language to define. Even your trivial example
added two new tags.

Btw. how do you pack a string as a member of a struct into a wl_array?
Strings are variable-size. Is it a pointer to char? Now libwayland
needs to learn to allocate and handle those, it's no longer just XML
syntax. C OTOH does not have the concept of variable-sized members, so
simply casting a pointer into a wl_array to a struct wouldn't work
like it does now with POD types.

> In case of real arrays, I'd imagine something like:
> 
> <arg name="reals" type="array" array="fixed"/>
> 
> And for "undescribed data" like you mention below,
> the "array" attribute can just be left out.

That I agree with. Maybe use "element_type" as the attribute to be very
explicit.

> No much more complicated than an enumeration.
> You could even nest structures, if needed.

> >> I am a language binding developer. In my C++ bindings [1] the developer
> >> currently has to know what the contents of a particular array are and do a
> >> manual cast (See the bottom of my README.md). This is sub-optimal.
> >> I'd rather have a std::vector<> (or whatever) with the correct type already
> >> in the argument list, otherwise the user could cast the array to the wrong
> >> thing accidentally.  
> >
> > That sounds like a very good use case. It made me rethink this a bit.
> >  
> >> > And yes, we have probably neglected to explicitly document the array
> >> > element type in all interfaces so far. That's a doc bug.  
> >>
> >> Are there any plans to fix that?  
> >
> > I don't. Maybe if I drive by something and notice, I might fix it, but
> > I don't have an intention to re-review everything.
> >
> > I think this is the point where "patches welcome" is a sincere suggestion.  
> 
> If I had more time on my hands, I'd love to improve that.
> Probably after my PhD thesis. ;)
> 
> To improve the documentation situation, you could from now on
> only accept protocol additions, when they are completly and
> properly documented, so that programmers don't have to look
> through lots of example code. I know, programmers don't like to
> write documentation, but it would certainly improve the situation.
> 
> Just a suggestion.

I think we try to do that by now, yes.

> >  
> >> Once the contents of arrays are _properly_ documented (complete type),
> >> language bindings could finally produce arrays with the correct content
> >> instead of having the user to manually cast them (possibly wrongly).  
> >
> > I would really prefer if you made the difference between documentation
> > and machine-readable description in terms.  
> 
> I'll try. But for an programmer there is really no difference between
> machine-readable and "human readable" documentation, because
> "machine-readable" doesn't mean it's not human readable. On the
> contrary, I most of the time prefer machine-readable documentation,
> because it is generally more precise and without room for interpretation.
> Therefore and since a am a language binding devoloper, I often value
> the machine-readable documation more.

For human-readable, one can invent new language and define terms on the
go and you can rely on people trying understand your explanation.

For machine-readable, one needs to first define the language rigorously
and unambiguously, and the you are stuck with that definition and
cannot express anything outside of it without defining new language and
updating tools to understand it. And not just the language but all the
terms, too. Plus, in our case we also need to guarantee
backward-compatiblity.

The difference in cost of writing is huge.


> > Adding machine-readable descriptions for the same POD types (int, uint,
> > fixed) would be feasible on one hand, but then we must also have the
> > escape hatch for "undescribed data, read the docs instead" since we
> > cannot get rid of that. It is also needed for XML files that have not
> > been fixed to have the new attribute.
> >
> > If we can describe the POD types, then we can also add the enum
> > attribute where it is applicable.
> >
> > Would that be workable for you?  
> 
> I don't really get what you are trying to say here. There is no need to add
> machine-readable descriptions for (u)int/fixed. It's already clear what they
> are. What we need are descriptions of the non-POD types: arrays.

That need is the ability to add the "element_type" attribute. That's
what I mean. You could use POD types in it and nothing else.

> I would however be fine with an "undescribed data" excaption if it's really
> neccessary. For exmaple if the type/structure of the contents can change
> from call to call (I think of pixel formats and audio right now).
> 
> >
> > Would anyone like to do the work?
> >
> > 1. Review all arrays in wayland and wayland-protocols and add any
> > missing documentation by cross-checking from well-known implementations.
> >
> > 2. Patch wayland-scanner to inspect the new array element type
> > attribute. Document the new attribute.
> >
> > 3. Patch wayland-scanner to inspect the enum attribute with arrays.
> > Update the XML language documentation.
> >
> > 4. Add array type and enum attributes to wayland and wayland-protocols
> > XML files.
> >
> > As for API and ABI stability, there will be no changes to the C
> > generator output, and generators for other languages get to solve their
> > own issues just like when we added the enum attribute. Ok?
> >
> > If someone wants, one could file a Phabricator Task and copy the
> > relevant bits from this email for tracking the development.
> >
> > Just to repeat, I don't think going beyond int, uint and fixed types is
> > a good idea.  
> 
> Well, arrays with arbitrary data in them are already new types.

No, they are not new types. They are the only array type existing so
far from the machine-readable i.e. XML language spec point of view.

> It's just a matter of documenting them. :)
> 
> >
> >
> > Thanks,
> > pq
> >
> >  
> >> [1] https://github.com/NilsBrause/waylandpp.  
> >  

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161125/a0a6b7fc/attachment.sig>

More information about the wayland-devel mailing list