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

[Danila Kutenin via Phabricator via libcxx-commits]

danlark added a comment.

All comments are done

I can also write a test for std::partition but maybe later



================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:38
+
+* Introduce new macro `_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY` which should
+  be a part of the libcxx config.
----------------
ldionne wrote:
> Can we rename this to `_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY` instead, since it is tied to the debug mode (if we apply my suggestions here)?
done


================
Comment at: libcxx/docs/UsingLibcxx.rst:244-250
+**_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY**:
+  This macro enables the randomization of unspecified behavior in libcxx, for
+  example, for equal elements in sorting algorithm or randomizing both parts of
+  the partition after `std::nth_element` call. This effort helps you to migrate
+  to potential future faster versions of these algorithms and deflake your tests
+  which depend on such behavior. To fix the seed, use
+  `_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY_SEED=seed` definition.
----------------
ldionne wrote:
> This should be moved to the documentation of the debug mode. In particular, I would like it if it were NOT advertised as something that can be tweaked by users. Users should only select the debug mode level.
Thanks, done


================
Comment at: libcxx/include/__config:900
 #endif
 
 #ifdef _LIBCPP_DISABLE_EXTERN_TEMPLATE
----------------
ldionne wrote:
> What would you think of enabling this automatically when the debug mode is enabled?
> 
> ```
> #if _LIBCPP_DEBUG_LEVEL >= 2
> # define _LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY
> #endif
> ```
I like it


================
Comment at: libcxx/include/algorithm:3150-3151
+template <class _Void>
+const void* const __random_unspecified_behavior_gen_<_Void>::__seed_static =
+    &__seed_static;
+
----------------
ldionne wrote:
> 
removed in favor of simpler version


================
Comment at: libcxx/include/algorithm:3153-3154
+
+using __random_unspecified_behavior_gen =
+    __random_unspecified_behavior_gen_<void>;
+
----------------
ldionne wrote:
> 
irrelevant, removed


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

https://reviews.llvm.org/D96946