[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:1682
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 typename enable_if
----------------
ldionne wrote:
> Does this break the library when used with the debug mode? There must have been a reason why we added `_LIBCPP_CONSTEXPR_IF_NODEBUG` in the first place?
> 
> @ericwf Do we care about this? If not, it looks like now's the time to remove the debug mode (for something better) as we discussed.
I think it should be the opposite: the tests fail in debug mode because this is no longer constexpr. I'll test it, though. 


================
Comment at: libcxx/include/algorithm:1894
+        return __move_constexpr(__first, __last, __result);
+#elif _LIBCPP_STD_VER > 17 // We must assume we are in a constexpr after C++17.
+    return __move_constexpr(__first, __last, __result);
----------------
ldionne wrote:
> I don't see the point of this `#elif`, can you explain?
After C++17 the standard says some users of this function should be constexpr so, we have to support that. But before C++20, if `_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED ` we probably want to take the fast path even if it means no constexpr. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721