maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vladislav!

On May 09, Vladislav Vaintroub wrote:
> >> +  void set_all()
> >> +  {
> >> +    set_prefix(width);
> >
> >may be better to optimize it?
> > memset(buffer, 0xff, sizeof(buffer) - 1);
> > ((uchar*)buffer)[sizeof(buffer)-1] = last_byte_mask(width);
> 
> I’m not really convinced this would be an optimization 😊
> My claim is that, since “width” is constant at compile time (128), and
> it is divisible by 32, any decent  C++ compiler must be able to figure
> out, at compile time, that set_all() ==
> set_prefix(width)==set_prefix(128) trivially reduces to a shorter

I hoped a compiler would optimize it, all constants are known at compile
time, indeed. But I didn't quite trust it to do so :)

But ok, whatever you prefer.

> >> +  {
> >> +    compile_time_assert(sizeof(ulonglong) == 8);
> >> +    uint32 tmp[2];
> >> +    int8store(tmp, map2buff);
> >> +    if (array_elements(buffer) > 2 && pad_with_ones)
> >> +      set_all();
> 
> >really? I'd expect that if pad_with_ones==true,
> >you simply don't need to do anything to the
> >rest of the buffer.
> 
> The original implementation does padding  with 0xff  or 0x00  ,
> depending on the highest order bit of map2buff, with
> bitmap_set_above(). retained that property

Oh. Could you please add a comment about this method's semantics?
Based just on the name, one can really expect that the Bitmap is
intersected with "ulonglong map2buff argument, padded with 0's or 1's
up to Bitmap's width".

And even with that semantics set_all() looks wrong. It should only set
the tail (bits above 64) to 1, not all bits.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx