This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Granularize algorithm includes
ClosedPublic

Authored by philnik on Feb 13 2022, 8:13 AM.

Details

Reviewers
Mordante
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Restricted Project
Commits
rG2e2f3158c604: [libc++] Granularize algorithm includes

Diff Detail

Event Timeline

philnik created this revision.Feb 13 2022, 8:13 AM
philnik requested review of this revision.Feb 13 2022, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2022, 8:13 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Seems fine to me if (1) CI is green and (2) we're all on board for breaking Hyrum's Law this month. I believe I've heard rumors to that effect — "it's very early in the 15.x release cycle, so let's remove all these redundant includes now and give people plenty of time to adjust and/or complain" — but I'll post in the Discord to get some nice explicit confirmation.

libcxx/test/std/containers/sequences/list/list.ops/sort.pass.cpp
47

Could drive-by fix this to std::distance while you're here, if you like...

Mordante accepted this revision as: Mordante.Feb 14 2022, 8:41 AM

Thanks for doing this! (It removes an item of my todo list :-) )
Two requests

  1. mention in the commit message that this removes several includes that people accidentally depend on. (in fact the addition in some headers show we already do this ourselves.) This isn't entirely obvious from the current message.
  2. mention the changes in the release notes. @ldionne do you want one note or should we mention all includes we removed?

LGTM, I leave the final approval this first cleanup for @ldionne.

libcxx/test/std/containers/sequences/list/list.ops/sort.pass.cpp
47

Slight preference to do that in a separate commit, just in case this one needs to be reverted.

ldionne requested changes to this revision.Feb 14 2022, 10:25 AM
ldionne added a subscriber: Restricted Project.

I am fine with this direction, however I agree CI should be green, and also we need to release-note this. I think the release note can only say that we removed transitive includes of <algorithm>, so if users see issues related to that, they know what the cause is. I don't think it's really useful to list all the headers that we removed <algorithm> *from*.

Requesting changes until those issues are fixed. Also, pinging libc++ vendors just so they are aware of this.

This revision now requires changes to proceed.Feb 14 2022, 10:25 AM
philnik updated this revision to Diff 408502.Feb 14 2022, 10:42 AM
  • Address comments; Fix CI
ldionne accepted this revision.Feb 14 2022, 3:12 PM

LGTM with fixed release note and green CI.

libcxx/docs/ReleaseNotes.rst
55
This revision is now accepted and ready to land.Feb 14 2022, 3:12 PM
philnik updated this revision to Diff 408665.Feb 14 2022, 4:15 PM
philnik marked an inline comment as done.
  • Rebased
  • Fix modules build
  • Address comment
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 4:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Feb 14 2022, 4:15 PM

The libcxxabi/test/test_demangle.pass.cpp change is adding #include <algorithm>. I don't know why phab wants this to be a binary.

philnik updated this revision to Diff 408885.Feb 15 2022, 8:25 AM
  • Fix Windows CI
philnik updated this revision to Diff 408919.Feb 15 2022, 9:19 AM
  • Try to fix Windows
ldionne accepted this revision.Feb 15 2022, 11:20 AM
This revision is now accepted and ready to land.Feb 15 2022, 11:20 AM
This revision was automatically updated to reflect the committed changes.