Hi,
On Mon, Nov 06, 2023 at 07:28:04AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> > This patch handles QAPI alternate types and generates data structures
> > in Go that handles it.
> >
> > Alternate types are similar to Union but without a discriminator that
> > can be used to identify the underlying value on the wire. It is needed
> > to infer it. In Go, most of the types [*] are mapped as optional
> > fields and Marshal and Unmarshal methods will be handling the data
> > checks.
> >
> > Example:
> >
> > qapi:
> > | { 'alternate': 'BlockdevRef',
> > | 'data': { 'definition': 'BlockdevOptions',
> > | 'reference': 'str' } }
> >
> > go:
> > | type BlockdevRef struct {
> > | Definition *BlockdevOptions
> > | Reference *string
> > | }
> >
> > usage:
> > | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
> > | k := BlockdevRef{}
> > | err := json.Unmarshal([]byte(input), &k)
> > | if err != nil {
> > | panic(err)
> > | }
> > | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
> >
> > [*] The exception for optional fields as default is to Types that can
> > accept JSON Null as a value. For this case, we translate NULL to a
> > member type called IsNull, which is boolean in Go. This will be
> > explained better in the documentation patch of this series but the
> > main rationale is around Marshaling to and from JSON and Go data
> > structures.
>
> These usage examples are great; in fact, I think they're too good to
> be relegated to the commit messages. I would like to see them in the
> actual documentation.
>
> At the same time, the current documentation seems to focus a lot on
> internals rather than usage. I think we really need two documents
> here:
>
> * one for users of the library, with lots of usage examples
> (ideally at least one for JSON->Go and one for Go->JSON for each
> class of QAPI object) and little-to-no peeking behind the
> curtains;
>
> * one for QEMU developers / QAPI maintainers, which goes into
> detail regarding the internals and explains the various design
> choices and trade-offs.
>
> Some parts of the latter should probably be code comments instead. A
> reasonable balance will have to be found.
>
> > diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> > +TEMPLATE_HELPER = """
> > +// Creates a decoder that errors on unknown Fields
> > +// Returns nil if successfully decoded @from payload to @into type
> > +// Returns error if failed to decode @from payload to @into type
> > +func StrictDecode(into interface{}, from []byte) error {
> > +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> > +\tdec.DisallowUnknownFields()
> > +
> > +\tif err := dec.Decode(into); err != nil {
> > +\t\treturn err
> > +\t}
> > +\treturn nil
> > +}
> > +"""
>
> I think the use of \t here makes things a lot less readable. Can't
> you just do
>
> TEMPLATE_HELPER = """
> func StrictDecode() {
> dec := ...
> if err := ... {
> return err
> }
> return nil
> }
> """
>
> I would actually recommend the use of textwrap.dedent() to make
> things less awkward:
>
> TEMPLATE_HELPER = textwrap.dedent("""
> func StrictDecode() {
> dec := ...
> if err := ... {
> return err
> }
> return nil
> }
> """
>
> This is particularly useful for blocks of Go code that are not
> declared at the top level...
>
> > + unmarshal_check_fields = f"""
> > +\t// Check for json-null first
> > +\tif string(data) == "null" {{
> > +\t\treturn errors.New(`null not supported for {name}`)
> > +\t}}"""
>
> ... such as this one, which could be turned into:
>
> unmarshal_check_fields = textwrap.dedent(f"""
> // Check for json-null first
> if string(data) == "null" {{
> return errors.New(`null not supported for {name}`)
> }}
> """)
>
> Much more manageable, don't you think? :)Didn't know this one, I agree 100% that is nicer. I'll take a look for the next iteration if the output would still be similar as I want to gofmt change be zero (see bellow). > > On a partially related note: while I haven't yet looked closely at > how much effort you've dedicated to producing pretty output, from a > quick look at generate_struct_type() it seems that the answer is "not > zero". I think it would be fine to simplify things there and produce > ugly output, under the assumption that gofmt will be called on the > generated code immediately afterwards. The C generator doesn't have > this luxury, but we should take advantage of it. Yes, I wholeheartedly agree. The idea of the generator producing a well formatted Go code came from previous review. I didn't want to introduce gofmt and friends to QEMU build system, perhaps it wasn't a big deal but I find it foreign to QEMU for a generated code that QEMU itself would not use. See: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01188.html Cheers, Victor
signature.asc
Description: PGP signature
