This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Keep unary_function and binary_function in C++17 for one more release
ClosedPublic

Authored by ldionne on Sep 22 2022, 1:22 PM.

Details

Reviewers
thieta
philnik
Mordante
Group Reviewers
Restricted Project
Summary

In LLVM 15, we added the deprecation markup for unary_function and
binary_function for >= C++11, and we also removed it for >= C++17.
While this is in accordance with the Standard, it's also a bit quick
for our users, since there was no release in which the classes were
marked as deprecated before their removal.

We noticed widespread breakage due to this, and after months of trying
to fix downstream failures, I am coming to the conclusion that users
will be better served if we give them one release where unary_function
is deprecated but still provided even in >= C++17.

Diff Detail

Event Timeline

ldionne created this revision.Sep 22 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 1:22 PM
ldionne requested review of this revision.Sep 22 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 1:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

If accepted, this patch would only go towards the LLVM 15 branch. The behavior for LLVM 16 is already the way we want it.

Note that I will be shipping this patch in Apple's libc++ no matter what we do here, simply because we've noticed too many failures caused by this and we need more time to address them. I think the greater community would also benefit from a 1-release grace period where we produce a deprecation warning but don't break people's code, but I can also live with the hard stance we've taken so far.

Mordante accepted this revision as: Mordante.Sep 23 2022, 11:38 AM
Mordante added a subscriber: Mordante.

If accepted, this patch would only go towards the LLVM 15 branch. The behavior for LLVM 16 is already the way we want it.

Note that I will be shipping this patch in Apple's libc++ no matter what we do here, simply because we've noticed too many failures caused by this and we need more time to address them. I think the greater community would also benefit from a 1-release grace period where we produce a deprecation warning but don't break people's code, but I can also live with the hard stance we've taken so far.

Mainly for my curiosity what is the issue to use _LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION at Apple?

If this is an issue for Apple, will that also be an issue for "less connected" companies? I mainly ask since I don't know how many other (large) companies are affected by this libc++ change. (I know Google likes to follow HEAD closely they are used to adapt to upstream changes quickly, other companies are probably not as agile.)

I love to deprecate things and remove cruft, but in the end libc++ should serve the needs of its customers. So if the current deprecation/removal policy is an issue, maybe we should reconsider our policy.

To me if this breaks Apple +1 to backport it for all our users.

LGTM I leave the final approval to @philnik .

libcxx/docs/ReleaseNotes.rst
147

This is the typical wording we use.

philnik accepted this revision.Sep 23 2022, 12:43 PM

LGTM with green CI and comments addressed.

libcxx/docs/ReleaseNotes.rst
147

Could we move the in the next release part into it's own sentence? Something like ... and above. This will be enforced in LLVM 16.. I find the current wording quite confusing.

libcxx/include/__functional/binary_function.h
26 ↗(On Diff #462278)

Can we get a message here? Something like binary_function will be removed in LLVM 16 from C++17 and above. The wording isn't great, but it's nicer than having to dig through release notes to find out. Same for unary_function.

This revision is now accepted and ready to land.Sep 23 2022, 12:43 PM
ldionne marked 3 inline comments as done.Sep 23 2022, 1:16 PM

If accepted, this patch would only go towards the LLVM 15 branch. The behavior for LLVM 16 is already the way we want it.

Note that I will be shipping this patch in Apple's libc++ no matter what we do here, simply because we've noticed too many failures caused by this and we need more time to address them. I think the greater community would also benefit from a 1-release grace period where we produce a deprecation warning but don't break people's code, but I can also live with the hard stance we've taken so far.

Mainly for my curiosity what is the issue to use _LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION at Apple?

If this is an issue for Apple, will that also be an issue for "less connected" companies? I mainly ask since I don't know how many other (large) companies are affected by this libc++ change. (I know Google likes to follow HEAD closely they are used to adapt to upstream changes quickly, other companies are probably not as agile.)

The issue is not to use _LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION itself. It's with coordinating tens of teams to do it. At the end of the day, it's much easier if folks get a deprecation warning and then fix it over the next ~N months at their own pace. If I try to put myself in someone else's shoes, they could be in a situation where they have slightly older dependencies (e.g. older open-source projects) that still use unary_function and binary_function. They then face the dilemma: either they can't compile as C++17, or they can't update to a new Clang/libc++, or they have to update their code and their dependencies to remove references to xx_function (or add _LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION to the build instructions). Depending on the setup, this can range from "trivial" (e.g. what I would imagine for Google) to annoying but achievable (for Apple) to unachievable in less than ~6 months (for IDK who). Since we don't lose very much, I think we might as well try to cater to everyone on that spectrum in this case.

I love to deprecate things and remove cruft, but in the end libc++ should serve the needs of its customers. So if the current deprecation/removal policy is an issue, maybe we should reconsider our policy.

I don't think our policy is an issue. I simply think we waited far too long to add the deprecation warnings for those two widely used types, and then we decided to both deprecate AND remove them in the same release. In hindsight, this was a poor decision and I think we should have deprecated it first, and then removed it. If anything is to change with our policy, I think it would be reasonable to never both deprecate and remove an API in a single release, since it can cause these issues. Of course, not all APIs are as widely used as binary_function and unary_function (yes, you'd be surprised how omnipresent they are).

libcxx/docs/ReleaseNotes.rst
147

Yeah, you're right. I reworked the wording.

libcxx/include/__functional/binary_function.h
26 ↗(On Diff #462278)

Upon considering this again, I think this part of the diff should not exist, just like it doesn't on our main branch.

ldionne updated this revision to Diff 462578.Sep 23 2022, 1:17 PM
ldionne marked 2 inline comments as done.

Address comments

IIRC this was closed right @ldionne so this patch can be closed?

ldionne closed this revision.Aug 31 2023, 6:50 AM

Yes, this was landed on release/15.x (and only there) as a cherry-pick as 8d802f78fa8c91707ecea1ae4ab141ba835429bc.