[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:1682
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 typename enable_if
----------------
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.


================
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);
----------------
I don't see the point of this `#elif`, can you explain?


================
Comment at: libcxx/include/algorithm:1958
 template <class _BidirectionalIterator1, class _BidirectionalIterator2>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _BidirectionalIterator2
----------------
This needs to be `constexpr` in C++20 and above only, since it's part of the API.


================
Comment at: libcxx/include/iterator:1376
+template <class _Ip, class _Op>
+_Op _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+copy(_Ip, _Ip, _Op);
----------------
Please keep the previous formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721