| Message ID | 1604646759-785-1-git-send-email-kaixuxia@tencent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Remove unnecessary conversion to bool | expand |
On Thu, Nov 5, 2020 at 11:12 PM <xiakaixu1987@gmail.com> wrote: > > From: Kaixu Xia <kaixuxia@tencent.com> > > Fix following warning from coccinelle: > > ./tools/lib/bpf/libbpf.c:1478:43-48: WARNING: conversion to bool not needed here > > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> > --- > tools/lib/bpf/libbpf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 313034117070..fb9625077983 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, > ext->name, value); > return -EINVAL; > } > - *(bool *)ext_val = value == 'y' ? true : false; > + *(bool *)ext_val = value == 'y'; I actually did this intentionally. x = y == z; pattern looked too obscure to my taste, tbh. > break; > case KCFG_TRISTATE: > if (value == 'y') > -- > 2.20.0 >
On Fri, 2020-11-06 at 13:32 -0800, Andrii Nakryiko wrote: > On Thu, Nov 5, 2020 at 11:12 PM <xiakaixu1987@gmail.com> wrote: > > Fix following warning from coccinelle: > > ./tools/lib/bpf/libbpf.c:1478:43-48: WARNING: conversion to bool not needed here [] > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c [] > > @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, > > ext->name, value); > > return -EINVAL; > > } > > - *(bool *)ext_val = value == 'y' ? true : false; > > + *(bool *)ext_val = value == 'y'; > > I actually did this intentionally. x = y == z; pattern looked too > obscure to my taste, tbh. It's certainly a question of taste and obviously there is nothing wrong with yours. Maybe adding parentheses makes the below look less obscure to you? x = (y == z); My taste would run to something like: --- tools/lib/bpf/libbpf.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 313034117070..5d9c9c8d50c9 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -1469,25 +1469,34 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, char value) { switch (ext->kcfg.type) { - case KCFG_BOOL: + case KCFG_BOOL: { + bool *p = ext_val; + if (value == 'm') { pr_warn("extern (kcfg) %s=%c should be tristate or char\n", ext->name, value); return -EINVAL; } - *(bool *)ext_val = value == 'y' ? true : false; + *p = (value == 'y'); break; - case KCFG_TRISTATE: + } + case KCFG_TRISTATE: { + enum libbpf_tristate *p = ext_val; + if (value == 'y') - *(enum libbpf_tristate *)ext_val = TRI_YES; + *p = TRI_YES; else if (value == 'm') - *(enum libbpf_tristate *)ext_val = TRI_MODULE; + *p = TRI_MODULE; else /* value == 'n' */ - *(enum libbpf_tristate *)ext_val = TRI_NO; + *p = TRI_NO; break; - case KCFG_CHAR: - *(char *)ext_val = value; + } + case KCFG_CHAR: { + char *p = ext_val; + + *p = value; break; + } case KCFG_UNKNOWN: case KCFG_INT: case KCFG_CHAR_ARR:
On Fri, Nov 6, 2020 at 1:50 PM Joe Perches <joe@perches.com> wrote: > > On Fri, 2020-11-06 at 13:32 -0800, Andrii Nakryiko wrote: > > On Thu, Nov 5, 2020 at 11:12 PM <xiakaixu1987@gmail.com> wrote: > > > Fix following warning from coccinelle: > > > ./tools/lib/bpf/libbpf.c:1478:43-48: WARNING: conversion to bool not needed here > [] > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > [] > > > @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, > > > ext->name, value); > > > return -EINVAL; > > > } > > > - *(bool *)ext_val = value == 'y' ? true : false; > > > + *(bool *)ext_val = value == 'y'; > > > > I actually did this intentionally. x = y == z; pattern looked too > > obscure to my taste, tbh. > > It's certainly a question of taste and obviously there is nothing > wrong with yours. > > Maybe adding parentheses makes the below look less obscure to you? > > x = (y == z); Yeah, I think this would be explicit enough. But let's keep the *(bool *) cast and keep switch code shorter and without extra {} block. > > My taste would run to something like: > --- > tools/lib/bpf/libbpf.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > [...]
From: Joe Perches > Sent: 06 November 2020 21:50 > > On Fri, 2020-11-06 at 13:32 -0800, Andrii Nakryiko wrote: > > On Thu, Nov 5, 2020 at 11:12 PM <xiakaixu1987@gmail.com> wrote: > > > Fix following warning from coccinelle: > > > ./tools/lib/bpf/libbpf.c:1478:43-48: WARNING: conversion to bool not needed here > [] > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > [] > > > @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, > > > ext->name, value); > > > return -EINVAL; > > > } > > > - *(bool *)ext_val = value == 'y' ? true : false; > > > + *(bool *)ext_val = value == 'y'; > > > > I actually did this intentionally. x = y == z; pattern looked too > > obscure to my taste, tbh. > > It's certainly a question of taste and obviously there is nothing > wrong with yours. > > Maybe adding parentheses makes the below look less obscure to you? > > x = (y == z); That just leads to people thinking conditionals need to be in parentheses and then getting the priorities for ?: all wrong as in: x = a + (b == c) ? d : e; It would (probably) be better to make 'ext_val' be a union type (probably a 'pointer to a union' rather than a union of pointers) so that all the casts go away. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 313034117070..fb9625077983 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc *ext, void *ext_val, ext->name, value); return -EINVAL; } - *(bool *)ext_val = value == 'y' ? true : false; + *(bool *)ext_val = value == 'y'; break; case KCFG_TRISTATE: if (value == 'y')