[libcxx-commits] [PATCH] D96946: [libcxx][RFC] Unspecified behavior randomization in libcxx

[Louis Dionne via Phabricator via libcxx-commits]

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

LGTM but we'll need to get CI to pass.



================
Comment at: libcxx/docs/ReleaseNotes.rst:54
 
+- ``_LIBCPP_DEBUG`` equals to ``1`` enables the randomization of unspecified behavior of standard algorithms (e.g. equal elements in ``std::sort`` or randomization of both sides of partition for ``std::nth_element``)
+
----------------
Let's break this line similarly to surrounding paragraphs.


================
Comment at: libcxx/include/__algorithm/nth_element.h:16-18
+#if defined(_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY)
+#  include <__algorithm/shuffle.h>
+#endif
----------------
Can you move this after the initial set of includes (on line 21 roughly) so that we can still keep the rest of the headers sorted easily? Applies everywhere.


================
Comment at: libcxx/include/__config:867-874
+#if defined(_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY)
+#  if defined(_LIBCPP_CXX03_LANG)
+#    error Support for unspecified stability is only for C++11 and higher
+#  endif
+#  define _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, __last) do { if (!__builtin_is_constant_evaluated()) _VSTD::shuffle(__first, __last, __libcpp_debug_randomizer()); } while (0)
+#else
+#  define _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, __last) (void)0
----------------
Personally I preferred the previous version where you had these checks inlined in the algorithms. I should have said it, sorry for the churn. Anyway, feel free to do it however you prefer.


================
Comment at: libcxx/include/__config:873
+#else
+#  define _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, __last) (void)0
+#endif
----------------
If you keep this, let's make it `do { } while (false)`.


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

https://reviews.llvm.org/D96946