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

Tue Nov 16 15:42:21 PST 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/include/__algorithm/shuffle.h:28

+#if defined(_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY) || defined(_LIBCPP_BUILDING_LIBRARY)
+
----------------
ldionne wrote:
> 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).
If we hadn't already decided to modularize everything regardless of motivation, then this would strike me as good motivation to split out a lightweight `<__random_base>`, and it could have been done right in this PR instead of being a project in itself. 😉 
(danlark: No action needed. :))

I guess I could volunteer to split up `<random>`... Once D110738 and D111514 land, I'll launch that project.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96946