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

Mon May 25 11:14:52 PDT 2020

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

================
Comment at: libcxx/include/algorithm:2504
 inline _LIBCPP_INLINE_VISIBILITY
-_RandomAccessIterator
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _RandomAccessIterator
 __rotate(_RandomAccessIterator __first, _RandomAccessIterator __middle, _RandomAccessIterator __last,
----------------
ldionne wrote:
> This can't be `constexpr` in C++14 since it uses `__rotate_forward`, which is only marked as `constexpr` starting in C++17 and above.
If it can take one of the first two paths, then it can be constexpr. I think it makes sense to support it in C++14 when possible. 

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp:133

+#if TEST_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED)
+    static_assert(test<input_iterator<const int*>, input_iterator<int*> >());
----------------
ldionne wrote:
> I would remove `&& !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED)` -- the test suite isn't supposed to have libc++ or compiler specific knowledge. If the test fails with some older compilers (e.g. clang 5), we can mark it as unsupported on that compiler.
`__builtin_is_constant_evalutated` is actually pretty new. It was only added to clang in the last year IIRC. I think this will be a long list of compilers. The clang that ships on the latest macOS (Apple clang 11) doesn't even support it. It may be worth adding something to test_macros. WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721