[libcxx-commits] [PATCH] D65721: Make rotate a constexpr
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Aug 5 09:30:28 PDT 2019
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/algorithm:2308
template <class _ForwardIterator>
-_ForwardIterator
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _ForwardIterator
__rotate_left(_ForwardIterator __first, _ForwardIterator __last)
----------------
This function can be made constexpr in all standard modes that support generalized constexpr (so C++14 and above).
That holds for all helper functions below.
================
Comment at: libcxx/include/algorithm:2367
inline _LIBCPP_INLINE_VISIBILITY
-_Integral
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _Integral
__algo_gcd(_Integral __x, _Integral __y)
----------------
Ditto.
================
Comment at: libcxx/include/algorithm:2381
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _RandomAccessIterator
__rotate_gcd(_RandomAccessIterator __first, _RandomAccessIterator __middle, _RandomAccessIterator __last)
{
----------------
Ditto.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate.pass.cpp:425
+template<class Iter>
+constexpr bool test_constexr()
+{
----------------
Typo: `test_constexpr`
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate.pass.cpp:458
+#if TEST_STD_VER > 17
+ static_assert(test_constexr<forward_iterator<int*> >());
+ static_assert(test_constexr<bidirectional_iterator<int*> >());
----------------
It would be nice if we could run all the same tests for constexpr and non-constexpr. It's not clear to me why we can't just run `test<....>` in a constexpr context. As for `test1`, it allocates memory but it might be possible to change the test to avoid that?
Generally speaking, I'd like for us to develop a fairly mechanical way of testing things in a constexpr context, since we're going to have to do that for almost all of `<algorithm>`. I think it's important to have the same test coverage for constexpr and non-constexpr (whenever it makes sense, of course).
Repository:
rCXX libc++
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65721/new/
https://reviews.llvm.org/D65721
More information about the libcxx-commits
mailing list