Message ID | 20190329200445.28512-1-chen.zhang@intel.com |
---|---|
State | New |
Headers | show |
Series | bitops.h: Remove unused bitops function test_and_change_bit() | expand |
About this patch, do we need merge similar function as one with return value? For example: test_and_set_bit()/set_bit(), test_and_clear_bit()/clear_bit(). Thanks Zhang Chen > -----Original Message----- > From: Zhang, Chen > Sent: Saturday, March 30, 2019 4:05 AM > To: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu- > devel@nongnu.org>; John Snow <jsnow@redhat.com>; Fam Zheng > <fam@euphon.net> > Cc: Zhang, Chen <chen.zhang@intel.com> > Subject: [PATCH] bitops.h: Remove unused bitops function > test_and_change_bit() > > From: Zhang Chen <chen.zhang@intel.com> > > In current codes we use change_bit() to finish the job. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > include/qemu/bitmap.h | 1 - > include/qemu/bitops.h | 15 --------------- > 2 files changed, 16 deletions(-) > > diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h index > 5c313346b9..6b71ef631c 100644 > --- a/include/qemu/bitmap.h > +++ b/include/qemu/bitmap.h > @@ -52,7 +52,6 @@ > * test_bit(bit, addr) Is bit set in *addr? > * test_and_set_bit(bit, addr) Set bit and return old value > * test_and_clear_bit(bit, addr) Clear bit and return old value > - * test_and_change_bit(bit, addr) Change bit and return old value > * find_first_zero_bit(addr, nbits) Position first zero bit in *addr > * find_first_bit(addr, nbits) Position first set bit in *addr > * find_next_zero_bit(addr, nbits, bit) Position next zero bit in *addr >= bit > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index > 3f0926cf40..1f98ffcdc0 100644 > --- a/include/qemu/bitops.h > +++ b/include/qemu/bitops.h > @@ -109,21 +109,6 @@ static inline int test_and_clear_bit(long nr, unsigned > long *addr) > return (old & mask) != 0; > } > > -/** > - * test_and_change_bit - Change a bit and return its old value > - * @nr: Bit to change > - * @addr: Address to count from > - */ > -static inline int test_and_change_bit(long nr, unsigned long *addr) -{ > - unsigned long mask = BIT_MASK(nr); > - unsigned long *p = addr + BIT_WORD(nr); > - unsigned long old = *p; > - > - *p = old ^ mask; > - return (old & mask) != 0; > -} > - > /** > * test_bit - Determine whether a bit is set > * @nr: bit number to test > -- > 2.17.GIT
On 3/29/19 4:04 PM, Zhang Chen wrote: > From: Zhang Chen <chen.zhang@intel.com> > > In current codes we use change_bit() to finish the job. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > include/qemu/bitmap.h | 1 - > include/qemu/bitops.h | 15 --------------- > 2 files changed, 16 deletions(-) > > diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h > index 5c313346b9..6b71ef631c 100644 > --- a/include/qemu/bitmap.h > +++ b/include/qemu/bitmap.h > @@ -52,7 +52,6 @@ > * test_bit(bit, addr) Is bit set in *addr? > * test_and_set_bit(bit, addr) Set bit and return old value > * test_and_clear_bit(bit, addr) Clear bit and return old value > - * test_and_change_bit(bit, addr) Change bit and return old value > * find_first_zero_bit(addr, nbits) Position first zero bit in *addr > * find_first_bit(addr, nbits) Position first set bit in *addr > * find_next_zero_bit(addr, nbits, bit) Position next zero bit in *addr >= bit > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > index 3f0926cf40..1f98ffcdc0 100644 > --- a/include/qemu/bitops.h > +++ b/include/qemu/bitops.h > @@ -109,21 +109,6 @@ static inline int test_and_clear_bit(long nr, unsigned long *addr) > return (old & mask) != 0; > } > > -/** > - * test_and_change_bit - Change a bit and return its old value > - * @nr: Bit to change > - * @addr: Address to count from > - */ > -static inline int test_and_change_bit(long nr, unsigned long *addr) > -{ > - unsigned long mask = BIT_MASK(nr); > - unsigned long *p = addr + BIT_WORD(nr); > - unsigned long old = *p; > - > - *p = old ^ mask; > - return (old & mask) != 0; > -} > - > /** > * test_bit - Determine whether a bit is set > * @nr: bit number to test > I personally don't see the harm in keeping this, but it is indeed unused, so: Reviewed-by: John Snow <jsnow@redhat.com> As for merging other sibling functions, I guess the desire is a small decrease in SLOC; I'm not sure if you'll run into any uses where the changed signatures for a combined function causes issues. I am not sure it's worth the hassle.
On Sat, 30 Mar 2019 at 03:09, Zhang Chen <chen.zhang@intel.com> wrote: > > From: Zhang Chen <chen.zhang@intel.com> > > In current codes we use change_bit() to finish the job. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > include/qemu/bitmap.h | 1 - > include/qemu/bitops.h | 15 --------------- > 2 files changed, 16 deletions(-) Do we gain anything by removing this function? IIRC these functions are all borrowed from the Linux kernel, so keeping the same API as the kernel does would make sense to me, even if we happen not to use all of it right now. thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Thursday, April 4, 2019 9:43 AM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu- > devel@nongnu.org>; John Snow <jsnow@redhat.com>; Fam Zheng > <fam@euphon.net> > Subject: Re: [Qemu-devel] [PATCH] bitops.h: Remove unused bitops function > test_and_change_bit() > > On Sat, 30 Mar 2019 at 03:09, Zhang Chen <chen.zhang@intel.com> wrote: > > > > From: Zhang Chen <chen.zhang@intel.com> > > > > In current codes we use change_bit() to finish the job. > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > include/qemu/bitmap.h | 1 - > > include/qemu/bitops.h | 15 --------------- > > 2 files changed, 16 deletions(-) > > Do we gain anything by removing this function? IIRC these functions are all > borrowed from the Linux kernel, so keeping the same API as the kernel does > would make sense to me, even if we happen not to use all of it right now. > Hi Peter, OK, I agree with you. But another question are the bitmap.h and bitops.h lack of maintenance. Please look at this patch: [PATCH] MAINTAINERS: Add unmaintained bitmap file to related field Do you have any comments? Thanks Zhang Chen > thanks > -- PMM
diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h index 5c313346b9..6b71ef631c 100644 --- a/include/qemu/bitmap.h +++ b/include/qemu/bitmap.h @@ -52,7 +52,6 @@ * test_bit(bit, addr) Is bit set in *addr? * test_and_set_bit(bit, addr) Set bit and return old value * test_and_clear_bit(bit, addr) Clear bit and return old value - * test_and_change_bit(bit, addr) Change bit and return old value * find_first_zero_bit(addr, nbits) Position first zero bit in *addr * find_first_bit(addr, nbits) Position first set bit in *addr * find_next_zero_bit(addr, nbits, bit) Position next zero bit in *addr >= bit diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index 3f0926cf40..1f98ffcdc0 100644 --- a/include/qemu/bitops.h +++ b/include/qemu/bitops.h @@ -109,21 +109,6 @@ static inline int test_and_clear_bit(long nr, unsigned long *addr) return (old & mask) != 0; } -/** - * test_and_change_bit - Change a bit and return its old value - * @nr: Bit to change - * @addr: Address to count from - */ -static inline int test_and_change_bit(long nr, unsigned long *addr) -{ - unsigned long mask = BIT_MASK(nr); - unsigned long *p = addr + BIT_WORD(nr); - unsigned long old = *p; - - *p = old ^ mask; - return (old & mask) != 0; -} - /** * test_bit - Determine whether a bit is set * @nr: bit number to test