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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/shuffle.h:28
 
+#if defined(_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY) || defined(_LIBCPP_BUILDING_LIBRARY)
+
----------------
danlark wrote:
> ldionne wrote:
> > danlark wrote:
> > > Question, windows DLLs were not able to build the tests without _LIBCPP_BUILDING_LIBRARY as I guess because of constructor/destructor ABI of trivial types, should I leave it like that?
> > > 
> > > Linux and MacOS seem fine without it
> > Oh, I actually meant to leave a comment here but forgot. Let's include this unconditionally, in other words remove the `#if defined` altogether.
> > 
> > When `_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY` is not used, this is simply going to be an unused type, which doesn't hurt. But at least we'll know it parses properly.
> Thanks!
Btw, I hope that the long-long-term plan is to split up `<random>` into `<__random/*.h>`, and then we could just `#include <__random/minstd_rand0.h>` and eliminate this type altogether. If you can think of a reason we //shouldn't// plan on doing that, this would be a good time to mention it.
(I guess one reason is that `minstd_rand0` comes with its own `operator<<`, which drags in `<iosfwd>`; but that's work-aroundable if we need to; we'd just split the `operator<<` stuff into its own header.)


================
Comment at: libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp:72
+  std::nth_element(snapshot_custom_v.begin(), snapshot_custom_v.begin() + kSize / 2, snapshot_custom_v.end(),
+                   [](const MyType& lhs, const MyType& rhs) { return lhs.value < rhs.value; });
+  bool all_equal = true;
----------------
This lambda is just `std::less<MyType>()`, right?


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

https://reviews.llvm.org/D96946