[libcxx-commits] [PATCH] D97365: [libc++] [C++2b] [P1682] Add to_underlying.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: libcxx/include/utility:1635
+__to_underlying(_Tp __val) noexcept {
+  return static_cast<underlying_type_t<_Tp> >(__val);
+}
----------------
Quuxplusone wrote:
> Mordante wrote:
> > 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.
> Your friendly neighborhood ADL-hater says: `_VSTD::__to_underlying`
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.


================
Comment at: libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This should be a `.verify.cpp` test instead!


================
Comment at: libcxx/test/std/utilities/utility/utility.underlying/to_underlying.fail.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++2a
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > curdeius wrote:
> > > > zoecarver wrote:
> > > > > Are we going to retire the `c++2a` alias at some point? Maybe this should be `c++20` instead. 
> > > > 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.
> > > 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.
> > 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.)
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97365