Remove misleading XOR

[Mark Wielaard]

Hi Timm,

On Thu, Feb 18, 2021 at 08:41:01AM +0100, Timm Bäder wrote:
> On 17/02/2021 17:49, Mark Wielaard wrote:
> > 
> > I think both the comment and the warning message are a little
> > misleading.
> > 
> > The comment should really read "Change state from CLEANING to
> > NO_RESIZING". You are right that NO_RESIZING is just zero, but still
> > removing it seems to make the code less clear.
> > 
> > All this is slightly confusing since it is doing "double" xor.
> > First it does an OLD_STATE_BITS ^ NEW_STATE_BITS and then a
> > atomic_fetch_xor of the result of that xor with the resizing_state
> > variable, Which is expected to be OLD_STATE, so the bits that are left
> > should be the NEW_STATE bits after the double xor.
> 
> This is how I understood the code. Given that all of the
> atomic_fetch_xor_explicit() calls in that file preceded by a comment
> explaining what they do, it might be useful to have a function
> change_resizing_state(htab, from, to, memory_order) instead and remove
> the comments. That also works around the clang warning of course, but
> those are not the only places where resizing_state is modified, so it's
> weird in a different way.
> 
> > Could you see if something like this works around the warning?
> > 
> > diff --git a/lib/dynamicsizehash_concurrent.c b/lib/dynamicsizehash_concurrent.c
> > index 2d53bec6..0283eea1 100644
> > --- a/lib/dynamicsizehash_concurrent.c
> > +++ b/lib/dynamicsizehash_concurrent.c
> > @@ -160,10 +160,10 @@ insert_helper (NAME *htab, HASHTYPE hval, TYPE val)
> >       }
> >   }
> > -#define NO_RESIZING 0u
> > -#define ALLOCATING_MEMORY 1u
> > -#define MOVING_DATA 3u
> > -#define CLEANING 2u
> > +#define NO_RESIZING       0x0
> > +#define ALLOCATING_MEMORY 0x1
> > +#define MOVING_DATA       0x3
> > +#define CLEANING          0x2
> 
> Nope, same result. :(

Hmmm. I am not sure why that doesn't work. What if you make them
explicitly unsinged (adding u at the end). Or does it simply ignore
the values and just warn because it sees the macro name and not an
explicit number?

I think I don't really understand anymore what this warning is warning
about.  Could you give an example of where this would flag something
that we would like to fix?

Maybe it is easiest to just suppress this warning, when is it enabled?

Thanks,

Mark