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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/algorithm:1873
 template <class _Tp, class _Up>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 typename enable_if
----------------
ldionne wrote:
> Wait, this doesn't work at all because `memmove` isn't `constexpr`. You still need the `_constexpr` functions, I just meant that it was reasonable to not try to use their `constexpr` version when `is_constant_evaluated()` isn't supported by the compiler.
> 
> I would expect that your tests fail with the patch as it is (and if not there's something I don't understand, so please LMK).
You're right. I was looking at an earlier version of the paper where memmove was constexpr and then ran the tests in C++17 mode (when the constexpr bits weren't enabled) and thought, "this is great!" 

Anyway, we could use `__builtin_memmove` after in Clang 8+ ([[ https://godbolt.org/z/5HE3Z5 | which is constexpr ]]).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721