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

From: Claudio DeSouza (cdesouza_at_[hidden])
Date: 2024-12-06 19:03:40

The compiler can still see the max value being passed in and your example
block has a fixed extent. By using first<> you get another span of fixed
extent.

This loop in general would benefit from being changed to something that
leverages the fixed extent even more. There are different approaches, but
ideally you want to iterate in a stride, which relies on the fixed extent
and always returned subspans of fixed extents each step, so you dispense
all the arithmetic access and subspan calls.

On Fri, 6 Dec 2024 at 18:56, Andrey Semashev via Boost <
boost_at_[hidden]> wrote:

> 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
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>