Remove misleading XOR

[Érico Nogueira]

Em 17/02/2021 13:49, Mark Wielaard escreveu:
> On Wed, 2021-02-17 at 11:12 +0100, Timm Bäder via Elfutils-devel wrote:
>> Hi,
>>
>> I'm mainly leaving this patch here to discuss what to do about it.
>> Clang complains because of -Wxor-used-as-pow. So of course one
>> possibility is to use -Wno-xor-used-as-pow. It also prints
>> '0x2 ^ NO_RESIZING' as an alternative to silence the warning. Given
>> the
>> comment just above the changed line, I think the 0x2 version is
>> better
>> than what I have in the patch now, since it still uses NO_RESIZING.
>> But
>> I'm open for suggestions on how to keep the code as clear as
>> possible.
> 
> 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.
> 
> The clang warning alternative is also slightly confusing. I think what
> it means to say is that it sees CLEANING is a constant int not
> expressed as a hex value. It really should show the definition:
> #define CLEANING 2u.
> 
> As far as I understand, if it sees a ^ xor on a hex value it knows you
> are dealing with bits, but if it sees it on a non-hex constant it isn't
> sure you are serious about xoring.
> 
> 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

Shouldn't you keep the explicit signedness of the constant? So 0x0u etc.

>   
>   #define STATE_BITS 2u
>   #define STATE_INCREMENT (1u << STATE_BITS)
> @@ -284,7 +284,7 @@ resize_master(NAME *htab)
>   
>     free(htab->old_table);
>   
> -  /* Change state to NO_RESIZING */
> +  /* Change state from CLEANING to NO_RESIZING */
>     atomic_fetch_xor_explicit(&htab->resizing_state, CLEANING ^ NO_RESIZING,
>                               memory_order_relaxed);
>   
>