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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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>
----------------
EricWF wrote:
> 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.
> Also this generates different ABI's between C++14 and C++17, which is a not OK.

I don't see it -- can you explain why we generate different ABIs? The goal of jumping through these hoops was specifically to generate the same ABI, but maybe we missed something (you know how tricky these changes are, so it's great to have additional opinions). This is how I understand the code right now (let's ignore `_LIBCPP_ABI_NO_BINDER_BASES` for the moment):

- `C++ <= 14`: we inherit from `unary_function<Arg, Result>`
- `C++ > 14`: we inherit from `__unary_function_keep_layout_base<Arg, Result>`

In both cases, the layout of the base classes is the same, and they have the same properties with respect to the empty base optimization applying. Did we miss something?



================
Comment at: libcxx/test/std/utilities/function.objects/func.require/binary_function.pass.cpp:13
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
EricWF wrote:
> 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.
Just to make sure I understand, are you suggesting that we drop the deprecation warnings from this patch entirely? If so, I'd disagree -- that's the whole point of the patch. I know it means we're requesting that users do some work, and that's not the most fun part of our job, but I think it's really important for the health of the ecosystem as a whole.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346