Re: [Qemu-devel] [PATCH 18/22] qidl: qidl.h

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, Jul 24, 2012 at 08:47:44PM +0000, Blue Swirl wrote:
> On Tue, Jul 24, 2012 at 5:20 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  qidl.h |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 qidl.h
> >
> > diff --git a/qidl.h b/qidl.h
> > new file mode 100644
> > index 0000000..75ae5af
> > --- /dev/null
> > +++ b/qidl.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * QEMU IDL Macros/stubs
> > + *
> > + * See docs/qidl.txt for usage information.
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Michael Roth    <mdroth@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPLv2.
> 
> GPLv2 or later?
> 

Yes, will fix.

> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef QIDL_H
> > +#define QIDL_H
> > +
> > +#include "qapi/qapi-visit-core.h"
> > +#include "qemu/object.h"
> > +#include "hw/qdev-properties.h"
> > +
> > +#ifdef QIDL_GEN
> > +
> > +/* we pass the code through the preprocessor with QIDL_GEN defined to parse
> > + * structures as they'd appear after preprocessing, and use the following
> > + * definitions mostly to re-insert the initial macros/annotations so they
> > + * stick around for the parser to process
> 
> I guess this doesn't work if the file #includes itself, like some
> flash files used to do (or I can't find the example just now).
> 

Yikes... I haven't encountered such a beast yet, but I don't see why it
wouldn't work? This series uses it for cases where annotations are pulled
in via #includes, so a file #include'ing itself should be any different,
as far as I can imagine.

> > + */
> > +#define QIDL(...) QIDL(__VA_ARGS__)
> > +#define QIDL_START(...) QIDL_START(__VA_ARGS__)
> > +#define QIDL_END(...) QIDL_END(__VA_ARGS__)
> > +
> > +#define QIDL_VISIT_TYPE(...)
> > +#define QIDL_SCHEMA_ADD_LINK(...)
> > +#define QIDL_PROPERTIES(...)
> 
> Maybe for this pass, the macros which do nothing like above could
> still do more syntax checking by specifying the macro argument list
> instead of ellipsis or even by introducing a dummy inline function.
> 
> > +
> > +#else /* !QIDL_GEN */
> > +
> > +#define QIDL(...)
> > +#define QIDL_START(name, ...)
> 
> Declare 'struct type of name' here (see below)?
> 

I'd like to keep it more general so we can do things like annotate
unions/enums/etc if need be, and also to be able to easily define
away all the QIDL annotations via configure to identity build
issues potentially caused by QIDL.

Not sure, but I think it may also cause some annoyances for cscope/ctags
users, or even just grepping for a type definition.

> > +#define QIDL_END(name) \
> > +    static struct { \
> 
> 'const', how about a name for the struct type too?
> 
> > +        void (*visitor)(Visitor *, struct name **, const char *, Error **); \
> 
> Here 'struct name **' could be const if the struct defined here is
> const. Probably 'struct name' should be declared earlier.
> 

Taking a 'const struct name **' and passing it in as a 'struct name**'?

If so, I think this comes down to enforcing non-const structs by having QIDL
generate the declaration, or just letting the build generate a warning. The
former is safer, but for the reasons above I'd prefer the latter since it
allows us to use plain C for type declarations.

> > +        const char *schema_json_text; \
> > +        Object *schema_obj; \
> > +        Property *properties; \
> > +    } qidl_data_##name;
> > +
> > +#define QIDL_VISIT_TYPE(name, v, s, f, e) qidl_data_##name.visitor(v, s, f, e)
> > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \
> > +    object_property_add_link(obj, path, "container", \
> > +                             &qidl_data_##name.schema_obj, errp);
> 
> The semicolons here and below seem useless.
> 
> > +#define QIDL_PROPERTIES(name) qidl_data_##name.properties;
> > +
> > +#endif /* QIDL_GEN */
> > +
> > +#endif
> > --
> > 1.7.9.5
> >
> >
>