This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Consistently replace `std::` qualification with `_VSTD::` or nothing. NFCI.
ClosedPublic

Authored by Quuxplusone on Nov 27 2020, 3:31 PM.

Details

Summary

I used a lot of git grep to find places where std:: was being used outside of comments and assert-messages. There were three outcomes:

  • Qualified function calls, e.g. std::move becomes _VSTD::move. This is the most common case.
  • Typenames that don't need qualification, e.g. std::allocator becomes allocator. Leaving these as _VSTD::allocator would also be fine, but I decided that removing the qualification is more consistent with existing practice.
  • Names that specifically need un-versioned std:: qualification, or that I wasn't sure about. For example, I didn't touch any code in <atomic>, <math.h>, <new>, or any ext/ or experimental/ headers; and I didn't touch any instances of std::type_info.

In some deduction guides, we were accidentally using class Alloc = typename std::allocator<T>, despite std::allocator<T>'s type-ness not being template-dependent. Because std::allocator is a qualified name, this did parse as we intended; but what we meant was simply class Alloc = allocator<T>.

Diff Detail

Event Timeline

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 27 2020, 3:31 PM
Quuxplusone requested review of this revision.Nov 27 2020, 3:31 PM
Quuxplusone edited the summary of this revision. (Show Details)Nov 27 2020, 3:31 PM
ldionne accepted this revision.Dec 1 2020, 2:09 PM

LGTM, thanks for increasing consistency.

I must admit I can't wait to reopen the discussion about replacing _VSTD:: by std::, though.

This revision is now accepted and ready to land.Dec 1 2020, 2:09 PM
mclow.lists added inline comments.
libcxx/include/memory
2086

This one may not be correct.
If a user has implemented an overload for std::swap for T1 or T2, then it's not clear to me that this wil find it.

Quuxplusone marked an inline comment as done.Dec 1 2020, 8:06 PM
Quuxplusone added inline comments.
libcxx/include/memory
2086

Hmm, that's quite true, this won't find a user-defined std::swap inserted manually into the outer std namespace. https://godbolt.org/z/vxKEcq
However... my change here is just bringing this call-site into line with the 20 other places in libc++ where we do the std::swap two-step. All 20 of them (and now, also this 21st) do using _VSTD::swap;. So if this one is now wrong, those other 20 are also wrong, and have been wrong for years.

Notably, std::pair's swap is implemented in terms of using _VSTD::swap; and not in terms of using std::swap;.

I think the correct response here is "Users shouldn't be adding their own functions into namespace std. They should be writing ADL swap functions instead."