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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF 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:
> ldionne wrote:
> > 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?
> > 
> Consider when `_LIBCPP_ABI_NO_BINDER_BASES` is defined. 
> It's observable if you're inheriting from `__unary_function_ebo_base` base or not.
> 
> Because we pass the extra arg to make sure the base class is unique, can't that uniqueness make something EBO away in C++17 that wouldn't in C++14?
> 
Though I grant the cases which trigger this are likely rare. But they're also the same set of cases this EBO optimization is targeting.

Please let me know if I'm misunderstanding something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346