Re: [PATCH 02/13] improve bitmap / sbitmap compatability of bitmap_set_bit

[Trevor Saunders]

On Wed, May 10, 2017 at 07:44:17AM +0100, Richard Sandiford wrote:
> tbsaunde+...@tbsaunde.org writes:
> > From: Trevor Saunders 
> >
> > This make the sbitmap version return true if the bit was previously
> > unset to make it similar to the bitmap version.
> >
> > gcc/ChangeLog:
> >
> > 2017-05-09  Trevor Saunders  
> >
> > * sbitmap.h (bitmap_set_bit): Return bool similar to bitmap
> > version of this function.
> > ---
> >  gcc/sbitmap.h | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/sbitmap.h b/gcc/sbitmap.h
> > index cba0452cdb9..d4e3177d495 100644
> > --- a/gcc/sbitmap.h
> > +++ b/gcc/sbitmap.h
> > @@ -108,11 +108,14 @@ bitmap_bit_p (const_sbitmap map, int bitno)
> >  
> >  /* Set bit number BITNO in the sbitmap MAP.  */
> >  
> > -static inline void
> > +static inline bool
> >  bitmap_set_bit (sbitmap map, int bitno)
> >  {
> > -  map->elms[bitno / SBITMAP_ELT_BITS]
> > -|= (SBITMAP_ELT_TYPE) 1 << (bitno) % SBITMAP_ELT_BITS;
> > +  SBITMAP_ELT_TYPE  = map->elms[bitno / SBITMAP_ELT_BITS];
> > +SBITMAP_ELT_TYPE mask = (SBITMAP_ELT_TYPE) 1 << (bitno) % 
> > SBITMAP_ELT_BITS;
> > +bool ret = (word & mask) == 0;
> > +word |= mask;
> > +return ret;
> >  }
> 
> Indentation looks off (mabye it's a mailer thing?).  Think the function
> comment should be updated too -- personally I can never remember whether
> true means "I just set it" or "it was already set" :-)

Sure, I can handle that.

> What's the current position on the use of references?  IMO a pointer
> is clearer here.

Well, I generally think non const references aren't a great idea, so I'm
not really sure why I used one here.  Anyway not a big deal so happy to
change that.

thanks

Trev

> 
> Thanks,
> Richard

Re: [PATCH 02/13] improve bitmap / sbitmap compatability of bitmap_set_bit

[Richard Sandiford]

tbsaunde+...@tbsaunde.org writes:
> From: Trevor Saunders 
>
> This make the sbitmap version return true if the bit was previously
> unset to make it similar to the bitmap version.
>
> gcc/ChangeLog:
>
> 2017-05-09  Trevor Saunders  
>
>   * sbitmap.h (bitmap_set_bit): Return bool similar to bitmap
> version of this function.
> ---
>  gcc/sbitmap.h | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/sbitmap.h b/gcc/sbitmap.h
> index cba0452cdb9..d4e3177d495 100644
> --- a/gcc/sbitmap.h
> +++ b/gcc/sbitmap.h
> @@ -108,11 +108,14 @@ bitmap_bit_p (const_sbitmap map, int bitno)
>  
>  /* Set bit number BITNO in the sbitmap MAP.  */
>  
> -static inline void
> +static inline bool
>  bitmap_set_bit (sbitmap map, int bitno)
>  {
> -  map->elms[bitno / SBITMAP_ELT_BITS]
> -|= (SBITMAP_ELT_TYPE) 1 << (bitno) % SBITMAP_ELT_BITS;
> +  SBITMAP_ELT_TYPE  = map->elms[bitno / SBITMAP_ELT_BITS];
> +SBITMAP_ELT_TYPE mask = (SBITMAP_ELT_TYPE) 1 << (bitno) % 
> SBITMAP_ELT_BITS;
> +bool ret = (word & mask) == 0;
> +word |= mask;
> +return ret;
>  }

Indentation looks off (mabye it's a mailer thing?).  Think the function
comment should be updated too -- personally I can never remember whether
true means "I just set it" or "it was already set" :-)

What's the current position on the use of references?  IMO a pointer
is clearer here.

Thanks,
Richard