On Thu, Sep 17, 2015 at 05:31:28PM +0200, Michal Privoznik wrote:
> So, now we have VIR_ENUM_DECL() and VIR_ENUM_IMPL() macros. They
> will define a pair of functions for you to convert from and to
> certain enum. Unfortunately, their usage is slightly cumbersome:
>
> int tmp;
> virMyAwesome ma;
>
> if ((tmp = virMyAwesomeTypeFromString(str)) < 0) {
> virReportError();
> goto cleanup;
> }
>
> ma = tmp;
>
> Ideally, we could avoid using the dummy @tmp variable:
>
> virMyAwesome ma;
>
> if (virMyAwesomeEnumFromString(str, &ma,
> "Unable to convert '%s'", str) < 0)
> goto cleanup;
>
> So, the first @str is string to convert from. @ma should point to
> the variable, where result is stored. Then, the third argument is
> the error message to be printed if conversion fails, followed by
> all the necessary arguments (message is in printf format).
I'm not seeing a hugely compelling reason to pass in the error
message string every time we parse an enum from a string. This
is no better than current code, where every caller has a potentially
different error message reported for the same scenario. I'd like
to see
virMyAwesome ma;
if (virMyAwesomeEnumFromString(str, &ma) < 0)
goto cleanup;
We could introduce an explicit VIR_ERR_INVALID_ENUM_VALUE
error code, and a fixed error message to go with it.
> -# define VIR_ENUM_DECL(name) \
> - const char *name ## TypeToString(int type); \
> - int name ## TypeFromString(const char*type);
> +# define VIR_ENUM_DECL(name) \
> + const char *name ## TypeToString(int type); \
> + int name ## TypeFromString(const char*type); \
> + int name ## EnumFromString(const char *str, int *result, \
> + const char *fmt, ...) \
> + ATTRIBUTE_FMT_PRINTF(3, 4);
It'd be nice to avoid generating two different FromString methods
at the same time too. I understand you probably did that to avoid
needing to convert all callers at once. How about we have a
VIR_ENUM_DECL_ERR macro generate the new format only, and just
convert all callers at a time, for any single enum.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list