[libcxx-commits] [PATCH] D65721: Make rotate a constexpr

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 4 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/algorithm:2308
 template <class _ForwardIterator>
-_ForwardIterator
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _ForwardIterator
 __rotate_left(_ForwardIterator __first, _ForwardIterator __last)
----------------
ldionne wrote:
> mclow.lists wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > 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.
> > > Was `std::rotate` updated as a paper or an issue? Or have we decided to extend constexpr support back to C++14 independent of the standard? 
> > We don't have the freedom to add `constexpr` to things in the standard.
> > We do have the freedom to add `noexcept`.
> > 
> > Odd, but that's the world we've got.
> My comment was only about helper functions. Make the helper functions constexpr whenever possible, but only mark the actual algorithm `std::rotate` constexpr when the Standard says so.
Ah, I see. Good call. Done.


================
Comment at: libcxx/include/algorithm:1862
 {
+#ifdef is_constant_evaluated
+    if (is_constant_evaluated())
----------------
I am concerned we are losing performance for those who do not have `is_constant_evaluated` and are not in a constexpr context. 


================
Comment at: libcxx/include/iterator:1536
     template <class _Tp>
-    _LIBCPP_CONSTEXPR_IF_NODEBUG friend
+    _LIBCPP_CONSTEXPR friend
     typename enable_if
----------------
What was the reasoning behind `_LIBCPP_CONSTEXPR_IF_NODEBUG`? I updated it to `_LIBCPP_CONSTEXPR` but, I want to make sure that is OK.


================
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*> >());
----------------
ldionne wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > 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).
> > I agree. What if we decided that we test constexpr by adding `return true` to the end of the test function and modifying the function to be a constexpr that returns a bool. This should work for almost all `<algorithm>` tests. 
> Yes, something along those lines is what I was thinking about. Can you see if that works?
It does.


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

https://reviews.llvm.org/D65721