[libcxx-commits] [PATCH] D134473: [libc++] Keep unary_function and binary_function in C++17 for one more release

Fri Sep 23 13:16:55 PDT 2022

ldionne marked 3 inline comments as done.
ldionne added a comment.

In D134473#3812335 <https://reviews.llvm.org/D134473#3812335>, @Mordante wrote:

> In D134473#3809725 <https://reviews.llvm.org/D134473#3809725>, @ldionne wrote:
>
>> 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).

================
Comment at: libcxx/docs/ReleaseNotes.rst:147
+  that this disables all deprecation warnings. Also note that per the Standard, ``unary_function``
+  and ``binary_function`` will be removed in C++17 and above starting in the next release.

----------------
philnik wrote:
> Mordante wrote:
> > This is the typical wording we use.
> 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.
Yeah, you're right. I reworked the wording.

================
Comment at: libcxx/include/__functional/binary_function.h:26
 {
-    typedef _Arg1   first_argument_type;
-    typedef _Arg2   second_argument_type;
-    typedef _Result result_type;
+    typedef _Arg1   first_argument_type _LIBCPP_DEPRECATED_IN_CXX17;
+    typedef _Arg2   second_argument_type _LIBCPP_DEPRECATED_IN_CXX17;
----------------
philnik wrote:
> 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`.
Upon considering this again, I think this part of the diff should not exist, just like it doesn't on our `main` branch.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134473/new/

https://reviews.llvm.org/D134473