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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

What a patch. Thanks a lot for your collaboration on all the iterations here, this is really not a simple one. LGTM once my comments are applied, but I'd like you to ping me once you land this so I can post-commit review the ABI test you added. Also, please make sure to get a green CI run.



================
Comment at: libcxx/include/__functional/binary_function.h:21
 
+#if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION)
+
----------------
Can you please add a comment in this file explaining what's going on? Point out the fact that we're jumping through these hoops to remain ABI compatible.


================
Comment at: libcxx/include/__functional/unary_function.h:12
 
 #include <__config>
 
----------------
For both `unary_function` and `binary_function`, I would like to add a test that would fail if this patch had the effect of changing the layout. You can base it on something like https://godbolt.org/z/5b3PnjK54, like `static_assert(sizeof(Foo) == X)`.


================
Comment at: libcxx/include/__functional/unary_function.h:20
 
+#if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION)
+
----------------
Same thing here, let's add a comment explaining the tricks we do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346