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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Your comment about loops made me think about generalized constexpr in C++14. Can you run the tests with your patch in C++11 and C++14 to make sure it works? I would expect failures due to the lack of generalized constexpr in C++11.



================
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
----------------
zoecarver wrote:
> zoecarver wrote:
> > 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. 
> The tests all pass with `_LIBCPP_DEBUG=1`. 
Can you try with `_LIBCPP_DEBUG=2`? It's a different debug level. I know it's kind of confusing.


================
Comment at: libcxx/include/algorithm:1863
 template <class _InputIterator, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _OutputIterator
----------------
zoecarver wrote:
> These all need to be `_LIBCPP_CONSTEXPR_AFTER_CXX17`. Otherwise, the compiler will complain about the loops in pre C++20 mode. 
I don't see why. Loops are allowed in constexpr since C++14. I guess we would need to use `_LIBCPP_CONSTEXPR_AFTER_CXX14` then.

Essentially, the rule I'm trying to go for is: make private helpers `constexpr` as often as they can be, and make public APIs `constexpr` when the standard requires them to.


================
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);
----------------
zoecarver wrote:
> 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?
Hmm, interesting. Yeah, that makes sense, however it also means that we might be pessimizing the runtime version in C++20 just because the compiler doesn't have `is_constant_evaluated`. I'm not 100% sure what the best approach is, however I would tend to say: In C++20, we are conforming w.r.t. constexpr algorithms only when the compiler supports `is_constant_evaluated`, otherwise we are non-conforming in favour of performance. In other words, when `is_constant_evaluated` isn't provided, we would not properly provide `constexpr` in these algorithms, and we'd always use the fast-but-not-constexpr version. This would avoid silent pessimizations, but would reduce our conformance level with older compilers. I don't think it's a huge deal because libc++ should normally be used with a recent compiler anyway, but this merits some thinking.

If we go down that route, we could simply use `__libcpp_is_constant_evaluated()` instead of the logic you have here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721