[libcxx-commits] [PATCH] D124346: [libc++] Complete the implementation of N4190

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

I'm pretty sure this change results in cases where, under ABI v2, C++14 and C++17 produce different ABI's. Am I mistaken?



================
Comment at: libcxx/include/__functional/unary_function.h:38
+
+template <class _EnsureUnique, class _Arg, class _Result> struct __unary_function_ebo_base {
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
----------------
As I mention below, I don't think this optimization is worth the cost. And I think adding this extra template parameter will increase debug info sizes enough to matter more than the types sizes. 


================
Comment at: libcxx/include/__functional/unary_function.h:51
+_LIBCPP_DIAGNOSTIC_POP
+#elif defined(_LIBCPP_ABI_NO_BINDER_BASES)
+template <class _Derived, class _Arg, class _Result>
----------------
I'm not convinced this optimization is worth the complexity it adds to the library. I suspect nobody is really writing code that would benefit from this, and if they are, I don't think they really care about the extra byte.

Also this generates different ABI's between C++14 and C++17, which is a not OK. You should be able to link code compiled in different dialects.


================
Comment at: libcxx/include/__functional/weak_result_type.h:59
     template <class _A1, class _A2, class _Rp>
-        static binary_function<_A1, _A2, _Rp>
-        __test(const volatile binary_function<_A1, _A2, _Rp>*);
+        static __binary_function_or_empty<__derives_from_binary_function<_Tp>, _A1, _A2, _Rp>
+        __test(const volatile __binary_function_or_empty<__derives_from_binary_function<_Tp>, _A1, _A2, _Rp>*);
----------------
This seems super weird and confusing. Does the thing derived from `std::binary_function` or not? Now we have weird cases.


================
Comment at: libcxx/test/std/utilities/function.objects/func.require/binary_function.pass.cpp:13
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Yeah, just disable the warning. It seems like a footgun to configure the library differently than the user would to test conforming behavior. The user shouldn't need to pass that flag, so neither should we.


================
Comment at: libcxx/test/std/utilities/function.objects/refwrap/weak_result.pass.cpp:17
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Or just turn off the warning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346