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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/algorithm:1892
 {
+#ifndef _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
+    if (__libcpp_is_constant_evaluated())
----------------
You don't need `#if _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED` anymore if you're using `__libcpp_is_constant_evaluated()` -- IIUC.


================
Comment at: libcxx/include/algorithm:2504
 inline _LIBCPP_INLINE_VISIBILITY
-_RandomAccessIterator
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _RandomAccessIterator
 __rotate(_RandomAccessIterator __first, _RandomAccessIterator __middle, _RandomAccessIterator __last,
----------------
zoecarver wrote:
> 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. 
Geez, you're right. I really glanced too quickly here.


================
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*> >());
----------------
zoecarver wrote:
> 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?
My preference would still be to say `// UNSUPPORTED: c++2a && apple-clang-11` (and the same for all unsupported compilers). The reason is that these `UNSUPPORTED` annotations are easy to get rid of as time goes by and we stop caring about older compilers. As such, it's easy-to-remove tech debt in the test suite.

On the other hand, if we engineer the test suite such that it can be run with compilers that don't support `is_constant_evaluated()`, we add complexity to the test suite itself, and removing it will require making sure that e.g. nobody's using the test suite with a compiler that doesn't support `is_constant_evaluated()`. Also, in theory, we should do the same for all features that involve compiler support, which isn't really scalable.

So.. my preference would be to be aggressive with compilers that don't support required features -- after all, libc++ is normally shipped alongside a matching Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721