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.
> >
> > 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...
> [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
