Re: Implement C++20 constexpr <algorithm>, , and <array>

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Mar 29, 2019 at 12:02:48PM -0400, Ed Smith-Rowland wrote:
> > > This differs from the previous patch in actually testing constexpr :-\ and
> > > in the addition of wrappers for __builtin_memmove and __builtin_memcmp that
> > > supply constexpr branches if C++20 and is_constant_evaluated().
> > +    void*
> > +    __memmove(_Tp* __dst, const _Tp* __src, ptrdiff_t __num)
> > +    {
> > +#if __cplusplus > 201703L
> > +      if (is_constant_evaluated())
> > +       {
> > +         for(; __num > 0; --__num)
> > +           {
> > +             *__dst = *__src;
> > +             ++__src;
> > +             ++__dst;
> > +           }
> > +         return __dst;
> > +       }
> > +      else if (__num)
> > +#endif
> > +       return __builtin_memmove(__dst, __src, sizeof(_Tp) * abs(__num));
> > +      return __dst;
> > ..
> > 	  const ptrdiff_t _Num = __last - __first;
> >            if (_Num)
> > -           __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
> > +           __memmove(__result, __first, _Num);
> > ..
> > 	  const ptrdiff_t _Num = __last - __first;
> >            if (_Num)
> > -           __builtin_memmove(__result - _Num, __first, sizeof(_Tp) * _Num);
> > +           __memmove(__result - _Num, __first, _Num);
> > 
> > Why the abs in there, that is something that wasn't previously there and
> > if the compiler doesn't figure out that __last >= __first, it would mean
> > larger emitted code for the non-constexpr case.  As memmove argument is
> > size_t, wouldn't it be better to make __num just size_t and remove this abs?
> > Also, wouldn't it be better to have on the other side the __num == 0
> > handling inside of __memmove, you already have it there for C++2a, but not
> > for older.  You could then drop the if (_Num) guards around __memmove.
> 
> memmove needs to be able to work with __last < __first also.

I don't get it, you are replacing calls with __builtin_memmove with
__memmove, and the __builtin_memmove calls didn't do anything like that,
the last argument is size_t and didn't use any abs.  So are you saying you
see crashes with the current code (when not in constexpr contexts) that your
patch fixes?

	Jakub