* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Hi
>
> ----- Original Message -----
> > Marc-André Lureau <marcandre.lur...@redhat.com> writes:
> >
> > > This will help with the introduction of a new structure to handle
> > > enum lookup.
It would be good to make that comment explain why it's necessary.
> > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > ---
> > [...]
> > > 45 files changed, 206 insertions(+), 122 deletions(-)
> >
> > Hmm.
> >
> > > diff --git a/include/qapi/util.h b/include/qapi/util.h
> > > index 7436ed815c..60733b6a80 100644
> > > --- a/include/qapi/util.h
> > > +++ b/include/qapi/util.h
> > > @@ -11,6 +11,8 @@
> > > #ifndef QAPI_UTIL_H
> > > #define QAPI_UTIL_H
> > >
> > > +const char *qapi_enum_lookup(const char * const lookup[], int val);
> > > +
> > > int qapi_enum_parse(const char * const lookup[], const char *buf,
> > > int max, int def, Error **errp);
> > >
> > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > index 4606b73849..c4f795475c 100644
> > > --- a/backends/hostmem.c
> > > +++ b/backends/hostmem.c
> > > @@ -13,6 +13,7 @@
> > > #include "sysemu/hostmem.h"
> > > #include "hw/boards.h"
> > > #include "qapi/error.h"
> > > +#include "qapi/util.h"
> > > #include "qapi/visitor.h"
> > > #include "qapi-types.h"
> > > #include "qapi-visit.h"
> > > @@ -304,7 +305,7 @@ host_memory_backend_memory_complete(UserCreatable *uc,
> > > Error **errp)
> > > return;
> > > } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
> > > error_setg(errp, "host-nodes must be set for policy %s",
> > > - HostMemPolicy_lookup[backend->policy]);
> > > + qapi_enum_lookup(HostMemPolicy_lookup, backend->policy));
> > > return;
> > > }
> > >
> >
> > Lookup becomes even more verbose.
> >
> > Could we claw back some readability with macros? Say in addition to
> >
> > typedef enum FOO {
> > ...
> > } FOO;
> >
> > extern const char *const FOO_lookup[];
> >
> > generate
> >
> > #define FOO_str(val) qapi_enum_lookup(FOO_lookup, (val))
> >
> > Needs a matching qapi-code-gen.txt update.
> >
> > With that, this patch hunk would become
> >
> > error_setg(errp, "host-nodes must be set for policy %s",
> > - HostMemPolicy_lookup[backend->policy]);
> > + HostMemPolicy_str(backend->policy);
> >
> > Perhaps we could even throw in some type checking:
> >
> > #define FOO_str(val) (type_check(typeof((val)), FOO) \
> > + qapi_enum_lookup(FOO_lookup, (val)))
> >
> > What do you think? Want me to explore a fixup patch you could squash
> > in?
> >
>
> Yes, I had similar thoughts, but didn't spent too much time finding the best
> proposal, that wasn't my main goal in the series.
>
> Indeed, I would prefer that further improvements be done as follow-up. Or if
> you have a ready solution, I can squash it there. I can picture the conflicts
> with the next patch though...
If you did it as a separate patch it would have to be another
huge patch changing lots of areas.
I think the use of the macro to keep it simple should be in this one;
you can add the type check change later.
Dave
> > [Skipping lots of mechanical changes...]
> >
> > I think you missed test-qobject-input-visitor.c and
> > string-input-visitor.c.
> >
> > In test-qobject-input-visitor.c's test_visitor_in_enum():
> >
> > v = visitor_input_test_init(data, "%s", EnumOne_lookup[i]);
> >
> > You update it in PATCH 12, but the code only works as long as EnumOne
> > has no holes. Mapping the enum to string like we do everywhere else in
> > this patch would be cleaner.
> >
>
> ok
>
> > The loop control also subscripts EnumOne_lookup[i]. You take care of
> > that one in PATCH 12. That's okay.
> >
> > Same for test-string-input-visitor.c's test_visitor_in_enum().
> >
> > There's one more in test_native_list_integer_helper():
> >
> > g_string_append_printf(gstr_union, "{ 'type': '%s', 'data': [ %s ] }",
> > UserDefNativeListUnionKind_lookup[kind],
> > gstr_list->str);
> >
> > Same story.
> >
> > The patch doesn't touch the _lookup[] subscripts you're going to replace
> > by qapi_enum_parse() in PATCH 07-11. Understand, but I'd reorder the
> > patches: first replace by qapi_enum_parse(), because DRY (no need to
> > explain more at that point). Then get rid of all the remaining
> > subscripting into _lookup[], i.e. this patch (explain it helps the next
> > one), then PATCH 12.
> >
>
> ok
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
