Re: [hash2][review] Early review (due to holidays)

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2024-12-06 18:55:55

On 12/6/24 21:46, Claudio DeSouza via Boost wrote:
> detail::read32le( block.subspan(i * 4).first<4>());
> 
> So you also get a sized span, and avoid checks.

I think, first() would need to check the dynamic span size anyway.

PS: Please, don't top-post.

> On Fri, 6 Dec 2024 at 17:42, Andrey Semashev via Boost <
> boost_at_[hidden]> wrote:
> 
>> On 12/6/24 20:30, Peter Dimov via Boost wrote:
>>> You can see practical manifestations of these examples in e.g
>>>
>>> https://github.com/pdimov/hash2/blob/develop/include/boost/hash2/md5.hpp
>>>
>>> where at
>>>
>>>
>> https://github.com/pdimov/hash2/blob/26b16023dc639ae354e246458719e4b569bc1d97/include/boost/hash2/md5.hpp#L110
>>>
>>> you have
>>>
>>>     BOOST_CXX14_CONSTEXPR void transform( unsigned char const block[ 64
>> ] )
>>>
>>> with the implicit precondition that `block` points at at least 64 valid
>> bytes,
>>> and then at
>>>
>>>
>> https://github.com/pdimov/hash2/blob/26b16023dc639ae354e246458719e4b569bc1d97/include/boost/hash2/md5.hpp#L119-L122
>>>
>>> you have
>>>
>>>         for( int i = 0; i < 16; ++i )
>>>         {
>>>             x[ i ] = detail::read32le( block + i * 4 );
>>>         }
>>>
>>> which relies on this implicit precondition to read 16 subspans of size 4.
>>
>> Assuming you had code like this:
>>
>>   void transform( span<unsigned char const, 64> block )
>>   {
>>     for( int i = 0; i < 16; ++i )
>>     {
>>       x[ i ] = detail::read32le( block.subspan(i * 4, 4) );
>>     }
>>   }
>>
>> I would presume, the subspan call would have a runtime check for the
>> offset and size. Then the converting constructor from span<const
>> unsigned char> returned from subspan to span<const unsigned char, 4>
>> accepted by read32le would have to check the size again. Yes, the
>> compiler could optimize those checks away, if it's smart enough. Or it
>> could not, e.g. if optimizations are disabled or it fails to propagate
>> constants for some reason. But none of these checks are actually
>> necessary, except maybe one at the entry, to enforce the size of the
>> entire block. Maybe not even that, if the callers of transform are
>> guaranteed to always pass the whole blocks.
>>
>>
>> _______________________________________________
>> Unsubscribe & other changes:
>> http://lists.boost.org/mailman/listinfo.cgi/boost
>>
> 
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost