This is an archive of the discontinued LLVM Phabricator instance.

[libc++] NFC: Move std::copy to its own header
AbandonedPublic

Authored by ldionne on May 28 2021, 9:48 AM.

Details

Reviewers
cjdb
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

ldionne created this revision.May 28 2021, 9:48 AM
ldionne requested review of this revision.May 28 2021, 9:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2021, 9:48 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This is an example patch for a mechanical split that we should do for all algorithms before we start doing ranges:: algorithms.

cjdb accepted this revision.May 28 2021, 9:58 AM
cjdb added a subscriber: cjdb.

Yeah, this is exactly what I'm doing, except on a bit more of a macro scale. I'll leave [alg.modifying.operations] in your hands?

I'd still like to see the long-term plan here: Would you have __algorithm/ranges_copy.h sitting alongside __algorithm/copy.h? Or would you roll them together into one header? (The former would proliferate headers twice as fast. But the latter would force all libc++ users of std::copy to depend on large swaths of <ranges>, so the former is the lesser evil.)

cjdb added a comment.May 28 2021, 2:41 PM

I'd still like to see the long-term plan here: Would you have __algorithm/ranges_copy.h sitting alongside __algorithm/copy.h? Or would you roll them together into one header? (The former would proliferate headers twice as fast.

We're planning to include std and ranges algorithms in the same header. That's why there's no __iterator/ranges_advance.h.

But the latter would force all libc++ users of std::copy to depend on large swaths of <ranges>, so the former is the lesser evil.)

This is objectively false. Users can only obtain std::copy by including <algorithm>, which imports "large swaths of <ranges>" in both cases.

But the latter would force all libc++ users of std::copy to depend on large swaths of <ranges>, so the former is the lesser evil.)

Users can only obtain std::copy by including <algorithm>

@cjdb, you misunderstand what I mean when I say "libc++ users of std::copy." Today, std::copy is used by libc++ in these headers:

$ git grep -l '_VSTD::copy('
__bit_reference
__string
algorithm
bitset
deque
locale
random
regex
valarray
vector

Whereas any part of <ranges> at all is included by only these headers:

$ git grep -l '<_*ranges' | grep -v ^__ranges
ranges
span
string_view
version

(where <span> and <string_view> include only enable_borrowed_range.h, and <version> is a false positive from a comment).
So, forcing all these libc++ users of _VSTD::copy to include large swaths of <ranges> would indeed be a tragedy.

(@ldionne, you understood my comment, right? or at the very least you understand it now?)

cjdb added a comment.May 28 2021, 3:17 PM

But the latter would force all libc++ users of std::copy to depend on large swaths of <ranges>, so the former is the lesser evil.)

Users can only obtain std::copy by including <algorithm>

@cjdb, you misunderstand what I mean when I say "libc++ users of std::copy." Today, std::copy is used by libc++ in these headers:

$ git grep -l '_VSTD::copy('
__bit_reference
__string
algorithm
bitset
deque
locale
random
regex
valarray
vector

Whereas any part of <ranges> at all is included by only these headers:

$ git grep -l '<_*ranges' | grep -v ^__ranges
ranges
span
string_view
version

(where <span> and <string_view> include only enable_borrowed_range.h, and <version> is a false positive from a comment).
So, forcing all these libc++ users of _VSTD::copy to include large swaths of <ranges> would indeed be a tragedy.

(@ldionne, you understood my comment, right? or at the very least you understand it now?)

Fair enough; I did misunderstand, and I agree that is worth thinking about. I don't consider it to be tragic (our fine-grained headers minimise the surface area of what's imported), but I can see why others might.
If we really need to split certain things, I guess that could come down to a case-by-case basis.

ldionne planned changes to this revision.Jun 1 2021, 7:00 PM

But the latter would force all libc++ users of std::copy to depend on large swaths of <ranges>, so the former is the lesser evil.)

So, forcing all these libc++ users of _VSTD::copy to include large swaths of <ranges> would indeed be a tragedy.

(@ldionne, you understood my comment, right? or at the very least you understand it now?)

Yes I do. IMO, the solution is to split headers not by their standard clause, but by what they represent logically. So, for example, we wouldn't put ranges::copy in __algorithm/copy.h, we'd put it in __ranges/copy.h (and similarly for everything else in ranges::). That gets rid of the issue.

But since we've been splitting by clause (which is OK too), I think we should go for __iterator/ranges_copy.h, or any other declination of that that does not bundle things from ranges:: with things outside of ranges::.

ldionne updated this revision to Diff 349163.Jun 1 2021, 7:06 PM

Rebase onto main

ldionne abandoned this revision.Jun 2 2021, 2:44 PM

Abandoning because Zoe's going to handle it.