> From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> Sent: Wednesday, 19 October 2022 15.48
>
> On Wed, Oct 19, 2022 at 09:37:02AM +0200, David Marchand wrote:
> > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> >
> > Signed-off-by: David Marchand <david.march...@redhat.com>
> > ---
>
> This patch looks pretty good to me. Some very small comments inline
> below.
> One thing I notice is that we are not supporting booleans except as
> part of
> an array or dictionary. Is it likely that we will ever want to have a
> telemetry command that just returns true/false alone? Don't see that
> being
> necessary just yet, so:
>
> Reviewed-by: Bruce Richardson <bruce.richard...@intel.com>
> Acked-by: Bruce Richardson <bruce.richard...@intel.com>
[...]
> > +/* Appends a boolean into the JSON array in the provided buffer. */
> > +static inline int
> > +rte_tel_json_add_array_bool(char *buf, const int len, const int
> used,
> > + bool val)
> > +{
> > + int ret, end = used - 1; /* strip off final delimiter */
> > + if (used <= 2) /* assume empty, since minimum is '[]' */
> > + return __json_snprintf(buf, len, "[%s]",
> > + val ? "true" : "false");
> > +
> > + ret = __json_snprintf(buf + end, len - end, ",%s]",
> > + val ? "true" : "false");
>
> Wonder if it's worthwhile doing a macro for this conditional, since the
> same ternary-operator snippet appears 4 times in this code.
A macro will probably degrade source code readability. Please keep as is.
I considered moving the conditional inside the format string to get rid of the
%s. But there is no performance requirement, and it is the exception that it
can be done for Booleans (but not integers and other types). So I concluded
that the current form is good.
