This is an archive of the discontinued LLVM Phabricator instance.

[libc++][nfc] remove duplicated __to_unsigned.
ClosedPublic

Authored by Mordante on May 12 2021, 8:52 AM.

Details

Summary

Both <type_traits> and <charconv> implemented this function with
different names and a slightly different behavior. This removes the
version in <charconv> and improves the version in <typetraits>.

  • The code can be used again in C++11.
  • The original claimed C++14 support, but [[nodiscard]] is not available in C++14.
  • Adds _LIBCPP_INLINE_VISIBILITY.

Diff Detail

Event Timeline

Mordante requested review of this revision.May 12 2021, 8:52 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 8:52 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb added a comment.EditedMay 12 2021, 9:07 AM

to-unsigned-like is named thusly because it works with make-unsigned-like-t, which is exposition-only transformation that could be make_unsigned_t or some unspecified thing if the type parameter isn't an integral. Even though we don't have any such types today, I'm not confident we won't in the future, and think both names should be kept.

LGTM % comments and buildkite failures. (Do you need to use _LIBCPP_CONSTEXPR and _NOEXCEPT on the definition? is that what buildkite is complaining about? I haven't looked)

libcxx/include/__ranges/size.h
92

IMO, please _VSTD:: this. (Yeah, its function parameter is a primitive type; but the fact that it has a function parameter is sufficient cause for me.)

libcxx/include/type_traits
2314–2320

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

cjdb added a subscriber: ldionne.May 12 2021, 9:40 AM
cjdb added inline comments.
libcxx/include/type_traits
2314–2320

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.

Mordante marked 3 inline comments as done.May 12 2021, 10:19 AM

Thanks for the reviews! It turns out our <type_traits> header is available in C++98. So I'll guard against this version.

libcxx/include/__ranges/size.h
92

I think it's indeed a bit pedantic, but we don't know the exact type of _Tp. In <charconv> I feel it's constrained so there it seems overkill.

libcxx/include/type_traits
2314–2320

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.

Mordante edited the summary of this revision. (Show Details)May 12 2021, 10:19 AM
Mordante updated this revision to Diff 344863.May 12 2021, 10:22 AM
Mordante marked 2 inline comments as done.
Mordante edited the summary of this revision. (Show Details)

Addresses the review comments:

  • Use the _like suffix since that's used in the Standard for ranges.
  • Adds an extra ADL protectsion.

The builds failed since <type_traits> is available in C++98.
Added guards against C++98 for the new function.

zoecarver accepted this revision as: zoecarver.May 12 2021, 10:52 AM

LGTM! Thanks! Feel free to approve this as libc++ yourself and land it. Or maybe wait to see if Arthur has any more comments.

libcxx/include/type_traits
2314–2318

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.

Quuxplusone accepted this revision.May 12 2021, 11:24 AM
Quuxplusone added inline comments.
libcxx/include/type_traits
2314–2318

Well, by "C++98" we mean "C++03" (they're basically synonymous), but sure, libc++ supports -std=c++03.

2314–2320

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.

This revision is now accepted and ready to land.May 12 2021, 11:24 AM
zoecarver added inline comments.May 12 2021, 11:30 AM
libcxx/include/type_traits
2314–2318

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 :)

zoecarver added inline comments.May 12 2021, 11:32 AM
libcxx/include/type_traits
2314–2320

FWIW +1 to naming this __to_unsigned (no _like). But that can also be done after the fact.

curdeius added inline comments.
libcxx/include/type_traits
2314–2318

@zoecarver, c++98 was a synonym for c++03 in libc++ tests and it was removed fairly recently (6 months ago or so).

Mordante marked 5 inline comments as done.May 12 2021, 12:04 PM
Mordante added inline comments.
libcxx/include/type_traits
2314–2318

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.

2314–2320

I'll ask @ldionne which name he prefers and then use that name.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.