On 12/30/22 23:33, Alejandro Colomar wrote:
> Hi Yann,
> 
> On 12/30/22 21:33, Alejandro Colomar wrote:
>> On 12/30/22 21:18, Yann Droneaud wrote:
>>> What's wrong with the following ?
> 
> [...]
> 
>>
>>>
>>>      unsigned long max = upper_bound - 1;
>>>      unsigned long mask = ULONG_MAX >> __builtin_clzl(max);
>>
>> I hate coding these magic operations out of a function, when I can give it a 
>> meaningful name.  That reads to me as a magic trick that many maintainers that 
>> read it after me will blame me for having to parse it.
>>
>> Moreover, it requires you to have the special case for 0 at the top, which I 
>> don't want.
> 
> I reconsidered; my -1 was equally magic.  And by calling it 'mask',
> ULONG_MAX >> n is something not so magic.
> 
> The builtin still has the problem that it requires special-casing 0, so I prefer 
> the C23 call, which provides the behavior I want for 0:
> 
> 
> unsigned long
> shadow_random_uniform(unsigned long upper_bound)
> {
>      unsigned long  r, max, mask;
> 
>      max = upper_bound - 1;
>      mask = ULONG_MAX >> leading_zerosul(max);

I just realized that this isn't good either.  Shifting right still has undefined 
behavior for max = 0.  I'll fall back to my original plan.

> 
>      do {
>          r = shadow_random();
>          r &= mask;  // optimization
>      } while (r > max);
> 
>      return r;
> }
> 
> 
> And, now I don't need to add a wrapper around bit_ceil() that removes the UB. 
> stdc_leading_zerosul() is just fine for this use case.
> 
> Cheers,
> 
> Alex
> 

-- 
<http://www.alejandro-colomar.es/>