RE: [PATCH] telemetry: support boolean type

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 19 October 2022 18.48
> 
> On Wed, Oct 19, 2022 at 04:28:58PM +0200, David Marchand wrote:
> > On Wed, Oct 19, 2022 at 3:48 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > 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.marchand@redhat.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.
> >
> > Err, naming it would be hard and I don't see for now how we could
> reuse it.
> >
> Yes, and I see Morten has objected from a readability perspective, so
> keeping as-is is fine.
> 
> One final suggestion though might be to have an array with the strings
> as so:
> 
> const char *bool_str[2] = { "false", "true" };
> 
> and then in the code use "bool_str[val]" in place of ternary operator.

I don't consider that more readable than the original.

> (From a quick check with godbolt is looks like bool params are clamped
> to 0
> or 1 on function call, but if we want to be paranoid, we can lookup
> based
> on [!!val])
> 
> However, ok to keep code as-is for this too.

Thank you, yes, please.

> 
> /Bruce