[libcxx-commits] [PATCH] D102332: [libc++][nfc] remove duplicated __to_unsigned.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/type_traits:2314
 
-template<class _From>
-[[nodiscard]] constexpr auto __to_unsigned_like(_From __x) noexcept {
-  return static_cast<make_unsigned_t<_From>>(__x);
+#ifndef _LIBCPP_CXX03_LANG
+template <class _Tp>
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > I don't quite understand this change? Maybe it comes from your comment:
> > > > The builds failed since <type_traits> is available in C++98.
> > > > Added guards against C++98 for the new function.
> > > 
> > > I thought we didn't support C++98 anymore. In any case, I strongly feel that we shouldn't. Do we have bots that run C++98? If so, I'm shocked they pass. 
> > > 
> > > 
> > Well, by "C++98" we mean "C++03" (they're basically synonymous), but sure, libc++ supports `-std=c++03`.
> That's not what I'm talking about. At one point we had `-std=c++98`, but I don't think we support that anymore. 
> 
> Anyway, ignore my original comment, I just misread the condition and thought you were enabling this starting in C++03 mode, which we wouldn't need a condition for. But we're not, so it's all good :)
I think we still support C++98/03. Some people still use it. It seems that the test `libcxx/modules/stds_include.sh.cpp` still runs in C++98 even on C++2b builds, see https://buildkite.com/llvm-project/libcxx-ci/builds/3205. And obviously C++98 doesn't support `constexpr` so the test failed.


================
Comment at: libcxx/include/type_traits:2314-2318
+template <class _Tp>
+_LIBCPP_NODISCARD_ATTRIBUTE _LIBCPP_INLINE_VISIBILITY constexpr
+typename make_unsigned<_Tp>::type __to_unsigned(_Tp __x) noexcept {
+    return static_cast<typename make_unsigned<_Tp>::type>(__x);
 }
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > cjdb wrote:
> > > > Quuxplusone wrote:
> > > > > Re naming: I approve of `__to_unsigned`. It simply converts to the `make_unsigned` version of the type, no more and no less.
> > > > > @cjdb, if libc++ ever adds support for user-defined "unsigned-like but not unsigned" types, we can revisit the naming bikeshed (since we'll have to re-audit all the call-sites anyway, it'll actually be //good// to force the pull-requester to touch them all).
> > > > If this were the case, then @ldionne wouldn't have asked for the concept `__integer_like`. At the very least, I think we should wait for Louis to chime in on this discussion.
> > > I dislike the `_like` suffix. But it seems ranges uses this suffix a lot. So for that reason I think adding the `_like` suffix is a better idea. 
> > I also dislike the `_like` suffix. "Ranges does it" is a great reason //not// to do it. :P So I'd like to see the shorter and less confusing (and less wishy-washy) name. But, I won't block this over it.
> FWIW +1 to naming this `__to_unsigned` (no `_like`). But that can also be done after the fact. 
I'll ask @ldionne which name he prefers and then use that name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102332