[libcxx-commits] [PATCH] D90569: [RFC] [libc++] P1645 constexpr for <numeric>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D90569#2371916 <https://reviews.llvm.org/D90569#2371916>, @Mordante wrote:

> I had another look at the header and noticed something odd for all overloads of `inclusive_scan` and `transform_inclusive_scan` and their signatures are odd
>
>   template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
>   _OutputIterator inclusive_scan(_InputIterator __first, _InputIterator __last,
>                                  _OutputIterator __result, _BinaryOp __b,  _Tp __init)
>
> Instead of the expected
>
>   template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
>   inline _LIBCPP_INLINE_VISIBILITY
>   _OutputIterator
>   inclusive_scan(_InputIterator __first, _InputIterator __last,
>                  _OutputIterator __result, _BinaryOp __b,  _Tp __init)
>
> Is there a reason why their signatures differ or is it an oversight?
> Shall I fix them in this patch?

I believe those are oversights, and the blame seems to confirm this. Good catch, and please fix them in this patch. You don't need `inline` though, just `_LIBCPP_INLINE_VISIBILITY`. These functions are already `inline` (i.e. linkonce-ODR) because they are templates.

On a different note, I would like to mention that I really like your approach of making sure everything's fine with a single function, and then going for the whole thing. I feel like this is a productive way to proceed, and I appreciate that.

Your approach LGTM, feel free to go ahead with the full thing!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90569/new/

https://reviews.llvm.org/D90569