This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove <functional> includes
ClosedPublic

Authored by philnik on Apr 20 2022, 2:00 PM.

Details

Reviewers
Mordante
var-const
ldionne
Group Reviewers
Restricted Project
Commits
rGa83f4b9cda57: [libc++] Remove <functional> includes

Diff Detail

Event Timeline

philnik created this revision.Apr 20 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 2:00 PM
Herald added a subscriber: miyuki. · View Herald Transcript
philnik requested review of this revision.Apr 20 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 2:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for the cleanup.

libcxx/docs/ReleaseNotes.rst
74

I feel this list is getting at the point were it becomes hard to read, and I expect the list to keep growing. I would suggest to change this entry to

- Some libc++ headers no longer transitively include all of
  
  - ``<algorithm>``,
  - ``<chrono>``,
  - ``<functional>``, and
  - ``<utility>``.

  If, after updating libc++, you see compiler errors related to missing declarations in namespace ``std``,
  it might be because one of your source files now needs to include one or more of the above headers.

(The formatting is untested.)

If you want to keep it as is, then please add Oxford comma's in both listings. (The first also misses a space.)

``<functional>``, and ``<utility>``. If, after updating libc++, you see compiler errors
philnik updated this revision to Diff 424564.Apr 22 2022, 12:14 PM
philnik marked an inline comment as done.
  • Update Release Notes
var-const accepted this revision as: var-const.Apr 22 2022, 9:13 PM
var-const added a subscriber: ldionne.

@philnik Can you add some more context to the patch and commit description? I think it would be useful to explain why it's safe to do this change now, at least.

LGTM, but because it can be a breaking change for users, please wait for @ldionne to review.

libcxx/docs/ReleaseNotes.rst
70–78

Nit: I think it would make sense to add a colon after of to precede the list.

71

Optional: I think it's more common for list elements to end in ; semicolons.

73

I'd remove this and -- it seems largely like a leftover from the previous state when this was a sentence and not a list.

philnik updated this revision to Diff 424695.Apr 23 2022, 1:07 AM
philnik marked 3 inline comments as done.
  • Address comments

@var-const What would you like to see in the description? The consensus was that we remove a few of the transitive includes in the libc++15 release cycle.
I don't think that Louis has to take a look here, since he approved of the direction. The entire reason this is in it's own patch is to be able to easily revert it just in case we break something in a really bad way.

libcxx/docs/ReleaseNotes.rst
71

I think it makes more sense to remove them entirely.

ldionne accepted this revision.Apr 25 2022, 2:09 PM
ldionne added a subscriber: Restricted Project.

LGTM. I understand that this will probably cause a bit of breakage, but it's trivial to fix and we have to get going if we ever want to clean up our transitive includes.

Pinging libc++ vendors for awareness.

This revision is now accepted and ready to land.Apr 25 2022, 2:09 PM
This revision was automatically updated to reflect the committed changes.