Re: [RFC PATCH 07/25] tools/xenbindgen: Add support for structs in TOML specs

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Mon Nov 25, 2024 at 3:03 PM GMT, Teddy Astie wrote:
> Hi,
>
> > +/// An IDL type. A type may be a primitive integer, a pointer to an IDL 
> > type,
> > +/// an array of IDL types or a struct composed of IDL types. Every integer 
> > must
> > +/// be aligned to its size.
> > +///
> > +/// FIXME: This enumerated type is recovered as-is from the `typ` field in 
> > the
> > +/// TOML files. Ideally, that representation should be more ergonomic and 
> > the
> > +/// parser instructed to deal with it.
> > +#[allow(clippy::missing_docs_in_private_items)]
> > +#[derive(Debug, serde::Deserialize, PartialEq)]
> > +#[serde(rename_all = "lowercase", tag = "tag", content = "args")]

FYI, this serde config is something I've also changed since the RFC. The
explicit tag and content are gone, because they complicate things and they are
actively harmful

Before (with explicit tag+content):

  typ = { tag = "u16" }
  typ = { tag = "struct", content = "foobar" }
  typ = { tag = "ptr", content = { tag = "u8" } }
  typ = { tag = "ptr", content = { tag = "struct", content = "foobar" } }

After (without explicit tag+content):

  typ = "u16"
  typ = { struct = "foobar" }
  typ = { ptr = "u8" }
  typ = { ptr = { struct = "foobar" } }

> > +pub enum Typ {
> > +    Struct(String),
> > +    U8,
> > +    U16,
> > +    U32,
> > +    U64,
> > +    I8,
> > +    I16,
> > +    I32,
> > +    I64,
> > +    Ptr(Box<Typ>),
> > +    Array(Box<Typ>, usize),
> > +}
> > +
>
> I think we can name it Type (it doesn't clash with a keyword actually)

Sure. I just wanted it to mirror the field name (because `type` is reserved).
In the big scheme of things it's not terribly important to do so.

>
> > +
> > +/// Deserialized form of a field within a hypercall struct (see 
> > [`StructDef`])
> > +#[derive(Debug, serde::Deserialize)]
> > +pub struct FieldDef {
> > +    /// Name of the field
> > +    pub name: String,
> > +    /// Description of what the field is for. This string is added as a 
> > comment
> > +    /// on top of the autogenerated field.
> > +    pub description: String,
> > +    /// Type of the field.
> > +    pub typ: Typ,
> > +}
> > +
>
> regarding this "typ" name, we can either use the "raw identifier" syntax 
> with r#type to have it "technically" named 'type' or use

I'd really rather not. Feels like playing with hot fire.

> #[serde(rename = "type")]
> to have it named "type" during deserialization even if the field is 
> still "typ"

This I like. I forgot I could do that.

>
> Cheers
> Teddy
>
>
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech

Cheers,
Alejandro