On Tue, May 02, 2017 at 04:37:03PM -0500, Eric Blake wrote:
> On 05/02/2017 03:31 PM, Eduardo Habkost wrote:
> > When parsing alternates from a string, there are some limitations in
> > what we can do, but it is a valid use case in some situations. We can
> > support booleans, integer types, and enums.
> >
> > This will be used to support 'feature=force' in -cpu options, while
> > keeping 'feature=on|off|true|false' represented as boolean values.
> >
> > Signed-off-by: Eduardo Habkost <[email protected]>
> > ---
>
> >
> > +/* Support for alternates on string-input-visitor is limited, because
> > + * the input string doesn't have any type information.
> > + *
> > + * Supported alternate member types:
> > + * 1) enums
> > + * 2) integer types
> > + * 3) booleans (but only if the there's no enum variant
> > + * containing "on", "off", "true", or "false" as members)
> > + *
> > + * UNSUPPORTED alternate member types:
> > + * 1) strings
> > + * 2) complex types
> > + */
> > +static void start_alternate(Visitor *v, const char *name,
> > + GenericAlternate **obj, size_t size,
> > + unsigned long supported_qtypes, Error **errp)
> > +{
> > + StringInputVisitor *siv = to_siv(v);
> > + QType t = QTYPE_QSTRING;
>
> Why do you document string as unsupported, and yet default to
> QTYPE_QSTRING? I don't see how a string is fundamentally different from
> an enum (no alternate can have both at the same time, an alternate with
> either type will have QTYPE_QSTRING set in supported_qtypes).
Strings are indistinguishable from enums inside
start_alternate(), that's true. But I wanted to explicitly
document 'str' as unsupported by string-input-visitor because
some string values would necessarily overlap with the string
representation of other variants.
>
> > +
> > + if (supported_qtypes & BIT(QTYPE_QBOOL)) {
> > + if (try_parse_bool(siv->string, NULL) == 0) {
> > + t = QTYPE_QBOOL;
> > + }
> > + }
> > +
> > + if (supported_qtypes & BIT(QTYPE_QINT)) {
> > + if (parse_str(siv, name, NULL) == 0) {
> > + t = QTYPE_QINT;
> > + }
> > + }
> > +
> > + *obj = g_malloc0(size);
> > + (*obj)->type = t;
>
> Should you raise an error if you couldn't match the input with
> supported_qtypes, rather than just blindly returning QTYPE_QSTRING?
The generated visitors calling visit_start_alternate() already
generate a (more specific and more useful) error message when
they see unsupported types.
--
Eduardo
