Page MenuHomePhabricator

[libc++] [C++2b] [P1682] Add to_underlying.
ClosedPublic

Authored by curdeius on Feb 24 2021, 1:35 AM.

Details

Diff Detail

Event Timeline

curdeius created this revision.Feb 24 2021, 1:35 AM
curdeius requested review of this revision.Feb 24 2021, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 1:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Feb 24 2021, 9:10 AM
libcxx/include/utility
1636

Would it be possible to make it available for earlier versions? I like to use this in D97115? I'm aware we're reluctant to offer features to earlier versions.

1638

The inline isn't needed.

libcxx/test/std/utilities/utility/utility.underlying/to_underlying.pass.cpp
24

Maybe interesting to also test these bounds for an plain enum.

28

This isn't used in the tests below.

libcxx/utils/generate_feature_test_macro_components.py
598

The paper has no number. Is it certain this will be the final value?

curdeius marked an inline comment as done.Feb 24 2021, 9:19 AM
curdeius added inline comments.
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.pass.cpp
24

Ok, will do.

28

It is in the last 3 assertions.

libcxx/utils/generate_feature_test_macro_components.py
598

I'll wait for the draft to be updated before committing I think to confirm the value.

Mordante added inline comments.Feb 24 2021, 10:11 AM
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.pass.cpp
28

A yes my bad.

curdeius updated this revision to Diff 326321.Feb 25 2021, 1:15 AM
curdeius marked an inline comment as done.

Address review comments:

  • Remove unnecessary inline.
  • Add plain enum tests.
  • Expose __to_underlying in C++14 onwards.
curdeius marked 4 inline comments as done.Feb 25 2021, 1:21 AM
curdeius added inline comments.
libcxx/include/utility
1636

Would this be OK?

libcxx/utils/generate_feature_test_macro_components.py
598

I'm pretty sure it will be the final value, since another feature approved in the same meeting uses this value. Cf. https://github.com/cplusplus/draft/pull/4515/files#diff-f46371f476893d323cfcb1383e9a70f07ff1c1f0c217e22294d0bf937aae6794R696.

curdeius marked an inline comment as done.Feb 25 2021, 1:30 AM
curdeius added inline comments.
libcxx/include/utility
1647

I wish we had using to_underlying = __to_underlying;.

Mordante accepted this revision.Feb 25 2021, 10:07 AM

One small request otherwise LGTM.

libcxx/include/utility
1640

Since we also use bitmask types in C++11, would it be possible to make this available in C++11? std::underlying_type is available in C++11.

libcxx/utils/generate_feature_test_macro_components.py
598

Sounds good enough to me. If it's wrong it's also trivial to fix.

Mordante added inline comments.Feb 25 2021, 10:10 AM
libcxx/utils/generate_feature_test_macro_components.py
598

I see there's a PR for this paper using 202102L so this looks good. https://github.com/cplusplus/draft/pull/4526/commits/47fd349eef8e4bdf5cdaed9513505f903d5e8f38

curdeius updated this revision to Diff 326613.Feb 25 2021, 11:39 PM
  • Expose __to_underlying in C++11 onwards.
curdeius marked an inline comment as done.Feb 25 2021, 11:39 PM
  • Expose __to_underlying in C++11 onwards.

Thanks!

zoecarver added inline comments.
libcxx/include/utility
1632

Nit: you could use underlying_type_t here.

libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

Are we going to retire the c++2a alias at some point? Maybe this should be c++20 instead.

curdeius marked an inline comment as done.Mar 4 2021, 1:22 AM
curdeius added inline comments.
libcxx/include/utility
1632

I can't, it's C++14 onwards.

#if _LIBCPP_STD_VER > 11
template <class _Tp> using underlying_type_t = typename underlying_type<_Tp>::type;
#endif
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

Hmm, when all compilers we support will accept -std=c++20?
IIUC, that's because lit std param is verified by injecting into -std=..., so we can't just use c++20 on a compiler that only accepts -std=c++2a.
Maybe there's some clever hack on lit params to translate c++20 into c++2a if the former isn't supported.
But I haven't looked into it.

Quuxplusone added inline comments.
libcxx/include/utility
1640

Your friendly neighborhood ADL-hater says: _VSTD::__to_underlying

libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

My (untested) impression is that

  • we cannot change only this c++2a to c++20, because this needs to exactly match llvm-lit's name --param std=c++2a
  • we SHOULD change llvm-lit to use --param std=c++20, AND to add --param std=c++2b at the same time; but this change is above my pay grade
  • one reason that change is difficult is because llvm-lit's --param std=c++XX relates, in some (non-obvious-to-me) way, to whether the actual compiler supports a -std=c++XX option. That entanglement SHOULD NOT exist, but it does. (We SHOULD just teach llvm-lit to map std=c++20 onto -std=c++{20,2a} depending on compiler support.)
  • and maybe the above change would require re-configuring or re-deploying buildkite and/or other infrastructure in ways that a simple "git push" wouldn't accomplish. I don't know and am scared to assume.
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.pass.cpp
62

I would be interested to see how std::to_underlying(s.bf) works on an enum bit-field. I know that's more of a core-language-trivia issue than a library-trivia issue, but it'd still be nice to have a test case.

And if you can construct a test case for the ADL issue (maybe involving enum X<Holder<Incomplete>>::E? I'd have to look up the rules again), that'd be cool (but not a blocker).

curdeius updated this revision to Diff 328221.Mar 4 2021, 10:00 AM
curdeius marked 2 inline comments as done.
  • Qualify.
  • Test bit-field enums.
curdeius updated this revision to Diff 328225.Mar 4 2021, 10:07 AM
  • Add ADL test.
curdeius marked an inline comment as done.Mar 4 2021, 10:08 AM
curdeius added inline comments.
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.pass.cpp
62

Done.
I've already started answering that I cannot find a test case for that and then I found it :).

curdeius marked an inline comment as done.Mar 4 2021, 10:10 AM
Mordante added inline comments.Mar 4 2021, 11:51 AM
libcxx/include/utility
1632

Nit: you could use underlying_type_t here.

I asked for C++11 support so I can use it in D97115.

libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

There's already support for c++2b. Personally I don't mind 2a in our tests and once all supported platforms support -std=c++20 we can change it in one go. Or would you also prefer to use 23 already? I get the impression the Standard committee feels more comfortable about their 3 year schedule and are already using C++23 instead of C++2b. (Note I'm not a member of the committee.)

Quuxplusone added inline comments.Mar 4 2021, 1:37 PM
libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

I think that llvm-lit should add support for --param std=c++20 on the same day that Clang adds support for -std=c++20. (This will require llvm-lit to understand that on certain kinds of buildbots, --param std=c++20 translates into -std=c++2a; but I think this must be a small change, because it already needs to understand that on certain kinds of buildbots, --param std=c++20 simply doesn't work at all.)

Severably, I think that Clang should add support for -std=c++2b on the same day that Clang adds support for -std=c++20.

Severably, I think that llvm-lit should add support for -std=c++2b on the same day that llvm-lit adds support for -std=c++20.

Of course the horse has already left the barn on all of these bullet points. But translate them all up by 3 (2a->2b, 20->23, 2b->2c) and they describe what I think should happen in the future. I think there should be a "deploy plan" for going from 17/2a, to 17/20/2b, to 17/20/23/2c, and each time it's executed it should be executed relatively atomically, without months or years of dangling loose ends. The situation described in https://stackoverflow.com/a/56483887/1424877 is pretty embarrassing for all three compiler vendors, IMO.

Code LGTM (modulo please remove the ADL test).
I'm not sure I understand the state of the world re: has this landed in C++2b yet? do we run C++2b tests yet? and is it March 8 yet? ;) so I don't want to be the one to say "accept". But I'm unlikely to have any further review comments, and certainly have no desire to block this if someone else wants to accept it.

libcxx/test/std/utilities/utility/utility.underlying/robust_against_adl.pass.cpp
42 ↗(On Diff #328225)

I don't think this is a reasonable thing to test; the user can't do this. (At least, if they do, we should laugh in their face.)

I believe I've convinced myself that there is no way to write a test for this VSTD-qualification. The only associated type of an enum is the-class-of-which-it-is-a-member, and that class must already be complete, by definition, so we can't use the Holder<Incomplete> trick to hard-error on completing it.
https://quuxplusone.github.io/blog/2019/04/26/what-is-adl/#for-example

Of course you should still do the VSTD-qualification, for consistent libc++ style if nothing else. But I strongly vote for removing this robust_against_adl test, because it does things (reopening namespace std) that the user really isn't allowed to do.

Code LGTM (modulo please remove the ADL test).
I'm not sure I understand the state of the world re: has this landed in C++2b yet? do we run C++2b tests yet? and is it March 8 yet? ;) so I don't want to be the one to say "accept". But I'm unlikely to have any further review comments, and certainly have no desire to block this if someone else wants to accept it.

So, yes we do test in C++2b mode, for a few weeks at least already.
And yes, it has been approved for C++2b. The draft is not yet updated, but there is a PR for that (https://github.com/cplusplus/draft/pull/4526).
And, I'll remove the ADL test.

libcxx/test/std/utilities/utility/utility.underlying/robust_against_adl.pass.cpp
42 ↗(On Diff #328225)

Agreed.

ldionne accepted this revision.Mar 4 2021, 2:59 PM

If the paper has been voted into the Standard, then I think it's fine for us to implement it. It seems to be the case, so let's ship it.

Apart from that and the other minor comments by reviewers, I think this is good to go.

libcxx/include/utility
1640

Just to be clear, we don't provide new features in older standard modes. It's fine to expose __to_underlying pre C++2b so you can use it to implement other stuff, but it has to come at no significant additional cost (OK here). But we *do not* expose std::to_underlying pre C++2b -- not sure which one you were asking about.

libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp
9 ↗(On Diff #326613)

The Lit issue will *soon* become moot once we stop supporting older compilers anyway. I suggest we don't do anything about this and just pinch our nose for a couple more months.

1 ↗(On Diff #328225)

This should be a .verify.cpp test instead!

This revision is now accepted and ready to land.Mar 4 2021, 2:59 PM
curdeius updated this revision to Diff 328409.Mar 4 2021, 11:48 PM
curdeius marked 3 inline comments as done.
  • Remove ADL test.
  • Rename fail -> verify.
libcxx/include/utility
1640

Yes, the idea was to expose __to_underlying (but not std::to_underlying) in pre C++2b, so that it can be used for bitmask types.

This revision was landed with ongoing or failed builds.Mar 5 2021, 1:31 AM
This revision was automatically updated to reflect the committed changes.
zoecarver added inline comments.Mar 5 2021, 9:33 PM
libcxx/include/utility
1632

Ah, thanks. Sorry, missed that.