Messages in this thread | | | From | Andrii Nakryiko <> | Date | Thu, 2 Jun 2022 22:33:54 -0700 | Subject | Re: [PATCH] libbpf: Fix is_pow_of_2 |
| |
On Thu, Jun 2, 2022 at 9:31 PM Zvi Effron <zeffron@riotgames.com> wrote: > > On Thu, Jun 2, 2022 at 9:17 PM Ian Rogers <irogers@google.com> wrote: > > > > From: Yuze Chi <chiyuze@google.com> > > > > There is a missing not. Consider a power of 2 number like 4096: > > > > x && (x & (x - 1)) > > 4096 && (4096 & (4096 - 1)) > > 4096 && (4096 & 4095) > > 4096 && 0 > > 0 > > > > with the not this is: > > x && !(x & (x - 1)) > > 4096 && !(4096 & (4096 - 1)) > > 4096 && !(4096 & 4095) > > 4096 && !0 > > 4096 && 1 > > 1 > > > > Reported-by: Yuze Chi <chiyuze@google.com> > > Signed-off-by: Yuze Chi <chiyuze@google.com> > > Signed-off-by: Ian Rogers <irogers@google.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 3f4f18684bd3..fd0414ea00df 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > > > static bool is_pow_of_2(size_t x) > > { > > - return x && (x & (x - 1)); > > + return x && !(x & (x - 1));
ugh... *facepalm*
> > No idea if anyone cares about the consistency, but in linker.c (same directory) > the same static function is defined using == 0 at the end instead of using the > not operator. > > Aside from the consistency issue, personally I find the == 0 version a little > bit easier to read and understand because it's a bit less dense (and a "!" next > to a "(" is an easy character to overlook). >
I agree, even more so, logical not used with arbitrary integer (not a pointer or bool) is a mental stumbling block for me, so much so that I avoid doing !strcmp(), for example.
But in this case, I'm not sure why I copy/pasted is_pow_of_2() instead of moving the one from linker.c into libbpf_internal.h as static inline. Let's do that instead?
> > } > > > > static size_t adjust_ringbuf_sz(size_t sz) > > -- > > 2.36.1.255.ge46751e96f-goog > >
| |