Re: [Qemu-devel] [PATCH] bitops.h: Remove unused bitops function test_and_change_bit()

Re: [Qemu-devel] [PATCH] bitops.h: Remove unused bitops function test_and_change_bit()

[John Snow]

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.

Re: [Qemu-devel] [PATCH] bitops.h: Remove unused bitops function test_and_change_bit()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] bitops.h: Remove unused bitops function test_and_change_bit()

[Zhang, Chen]

> -----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