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
