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

[Zoe Carver via Phabricator via libcxx-commits]

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


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


================
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:
> 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.
Fair enough. I'll use `UNSUPPORTED`.

Why the `C++2a` part, though? I think I'll leave the `TEST_STD_VER > 17` condition because that's what the standard says so, we shouldn't need the `C++2a` condition.

Also, I think we should support `clang-8` and up. What are the corresponding `apple-clang` versions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721