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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/shuffle.h:28
 
+#if defined(_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY) || defined(_LIBCPP_BUILDING_LIBRARY)
+
----------------
Quuxplusone wrote:
> 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.)
Ah, so you do like the modularization of headers, then? 😄

We can address this later when we modularize `<random>` (yes, that is the long term goal).


================
Comment at: libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp:18-22
+#include <algorithm>
+#include <array>
+#include <iterator>
+#include <cassert>
+#include <vector>
----------------
Just for posterity, we try to keep includes sorted. Fixed upon committing.


================
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;
----------------
Quuxplusone wrote:
> This lambda is just `std::less<MyType>()`, right?
Fixed upon committing.


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

https://reviews.llvm.org/D96946