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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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.



================
Comment at: libcxx/test/std/utilities/utility/utility.underlying/robust_against_adl.pass.cpp:42
+struct underlying_type<Tester<T> > : underlying_type<E> {};
+} // namespace std
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97365