This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [test] Qualify `distance` as `std::distance` in a lot of tests. NFCI.
ClosedPublic

Authored by Quuxplusone on Feb 13 2022, 7:03 PM.

Details

Summary

We shouldn't be calling distance via ADL -- and neither should anybody in the wild be calling it via ADL, so it's not like we need to test this ADL ability of distance in particular.

This and D119677 + D119687 came out of my running the tests with a modified version of D119670 to look for all the places we call std:: functions unqualified. I claim that almost every time we do this, it's either accidental or unwise or both.

If this std::distance patch looks good to folks, then I've got similar patches lined up for

  • std::next
  • std::prev
  • std::move
  • std::getline
  • std::atomic_flag_{clear,test}
  • iomanip functions (std::hex, std::showpoint, et al) in std/localization/
  • iomanip functions (std::hex, std::showpoint, et al) in std/input.output/iostream.format/
  • using std::optional; using std::make_optional; in std/utilities/optional/
  • using std::any; using std::any_cast; in std/utilities/any/ (this one is not purely mechanical, though)

where I propose to post at least the first 8 as one giant PR just to poke CI, and then land it as 8 individual commits if CI is green. @ldionne @Mordante any objections to that plan?

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 13 2022, 7:03 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 13 2022, 7:03 PM
Quuxplusone edited the summary of this revision. (Show Details)Feb 13 2022, 8:37 PM
ldionne accepted this revision.Feb 14 2022, 12:45 PM

This patch and the proposed plan SGTM, however if all the changes look basically like this (just adding std:: mechanically), I'd rather just have all the follow-ups in the same PR. It's going to be as easy to review and will create less review traffic.

This revision is now accepted and ready to land.Feb 14 2022, 12:45 PM
Mordante accepted this revision.Feb 14 2022, 1:08 PM

This patch and the proposed plan SGTM, however if all the changes look basically like this (just adding std:: mechanically), I'd rather just have all the follow-ups in the same PR. It's going to be as easy to review and will create less review traffic.

+1 to what Louis said.

Thanks for the cleanup and LGTM.