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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 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:
> 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.
All tests pass :)


================
Comment at: libcxx/include/algorithm:1863
 template <class _InputIterator, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _OutputIterator
----------------
ldionne wrote:
> 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.
I didn't think to check, you're right loops in constexpr in C++14 work fine. 

> 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.

Great, I think this is a good idea. 


================
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:
> 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.
My thinking here is that it's doubtful (especially at this point in time) that someone will be enabling C++20 on a compiler that isn't fairly recent. 

But, this is actually a moot point because I just realized after P0202 we can use `memmove` in a constexpr context. So, we can remove `__move_constexpr` altogether. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65721