Tighten absl::BitGenRef binding rules and simplify its implementation. * Reject binding to const generators. Generating a variate mutates the generator, but BitGenRef previously accepted `const URBG&` and later invoked the non-const `operator()` through a cast, which is undefined behavior for generators defined const. Such constructions now resolve to a deleted constructor. * Detect `operator BitGenRef()` on an lvalue rather than an rvalue. This lets types ref-qualify the operator (`operator BitGenRef() &`) to prevent binding a BitGenRef to a temporary; previously such types were silently wrapped as a URBG instead, adding a layer of indirection. * Merge the mocking and non-mocking constructors; the mock dispatch function is now selected with `if constexpr`. * Store the generator as `void*` instead of `uintptr_t`, replacing reinterpret_casts with static_casts, and use a null mock function pointer instead of a `NotAMock` sentinel. PiperOrigin-RevId: 1004460719
diff --git a/absl/random/bit_gen_ref.h b/absl/random/bit_gen_ref.h index df8675b..9f7cdcc 100644 --- a/absl/random/bit_gen_ref.h +++ b/absl/random/bit_gen_ref.h
@@ -26,6 +26,7 @@ #include <cstdint> #include <limits> +#include <memory> #include <type_traits> #include "absl/base/attributes.h" @@ -69,13 +70,25 @@ struct detector<Trait, std::void_t<Trait<Args...>>, Args...> : std::true_type {}; + // Detects a conversion operator callable on an lvalue, so that types may + // ref-qualify it (`operator BitGenRef() &`) to prevent binding to + // temporaries. template <typename T> using has_conversion_operator_t = - decltype(std::declval<T>().operator BitGenRef()); + decltype(std::declval<T&>().operator BitGenRef()); template <typename T> using HasConversionOperator = detector<has_conversion_operator_t, void, T>; + // True when BitGenRef binds directly to a URBG, rather than copying + // (BitGenRef and derived types) or converting (types which provide + // `operator BitGenRef()`). + template <typename URBG> + static constexpr bool kBindsDirectly = + !std::is_same_v<URBG, BitGenRef> && !std::is_base_of_v<BitGenRef, URBG> && + !HasConversionOperator<URBG>::value && + random_internal::is_urbg<URBG>::value; + public: BitGenRef(const BitGenRef&) = default; BitGenRef(BitGenRef&&) = default; @@ -84,27 +97,20 @@ template <typename URBGRef, typename URBG = absl::remove_cvref_t<URBGRef>, typename std::enable_if_t< - (!std::is_same_v<URBG, BitGenRef> && - !std::is_base_of_v<BitGenRef, URBG> && - !HasConversionOperator<URBG>::value && - random_internal::is_urbg<URBG>::value && - !RandomMockingAccess::HasInvokeMock<URBG>::value)>* = nullptr> + kBindsDirectly<URBG> && + !std::is_const_v<std::remove_reference_t<URBGRef>>>* = nullptr> BitGenRef(URBGRef&& gen ABSL_ATTRIBUTE_LIFETIME_BOUND) // NOLINT - : t_erased_gen_ptr_(reinterpret_cast<uintptr_t>(&gen)), - mock_call_(NotAMock), + : t_erased_gen_ptr_(std::addressof(gen)), + mock_call_(MockCallFn<URBG>()), generate_impl_fn_(ImplFn<URBG>) {} + // Generating a variate mutates the generator, so a BitGenRef may not bind to + // a const generator. template <typename URBGRef, typename URBG = absl::remove_cvref_t<URBGRef>, typename std::enable_if_t< - (!std::is_same_v<URBG, BitGenRef> && - !std::is_base_of_v<BitGenRef, URBG> && - !HasConversionOperator<URBG>::value && - random_internal::is_urbg<URBG>::value && - RandomMockingAccess::HasInvokeMock<URBG>::value)>* = nullptr> - BitGenRef(URBGRef&& gen ABSL_ATTRIBUTE_LIFETIME_BOUND) // NOLINT - : t_erased_gen_ptr_(reinterpret_cast<uintptr_t>(&gen)), - mock_call_(MockCall<URBG>), - generate_impl_fn_(ImplFn<URBG>) {} + kBindsDirectly<URBG> && + std::is_const_v<std::remove_reference_t<URBGRef>>>* = nullptr> + BitGenRef(URBGRef&& gen) = delete; // NOLINT using result_type = uint64_t; @@ -119,35 +125,41 @@ result_type operator()() { return generate_impl_fn_(t_erased_gen_ptr_); } private: - using impl_fn = result_type (*)(uintptr_t); - using mock_call_fn = bool (*)(uintptr_t, FastTypeIdType, void*, void*); + using impl_fn = result_type (*)(void*); + using mock_call_fn = bool (*)(void*, FastTypeIdType, void*, void*); template <typename URBG> - static result_type ImplFn(uintptr_t ptr) { + static result_type ImplFn(void* ptr) { // Ensure that the return values from operator() fill the entire // range promised by result_type, min() and max(). absl::random_internal::FastUniformBits<result_type> fast_uniform_bits; - return fast_uniform_bits(*reinterpret_cast<URBG*>(ptr)); + return fast_uniform_bits(*static_cast<URBG*>(ptr)); } // Get a type-erased InvokeMock pointer. template <typename URBG> - static bool MockCall(uintptr_t gen_ptr, FastTypeIdType key_id, - void* args_tuple, void* result) { - return RandomMockingAccess::InvokeMock(reinterpret_cast<URBG*>(gen_ptr), - key_id, args_tuple, result); - } - static bool NotAMock(uintptr_t, FastTypeIdType, void*, void*) { - return false; + static bool MockCall(void* gen_ptr, FastTypeIdType key_id, void* args_tuple, + void* result) { + return RandomMockingAccess::InvokeMock(static_cast<URBG*>(gen_ptr), key_id, + args_tuple, result); } - inline bool InvokeMock(FastTypeIdType key_id, void* args_tuple, - void* result) { - if (mock_call_ == NotAMock) return false; // avoids an indirect call. + // Returns MockCall<URBG> if URBG supports mocking, otherwise nullptr. + template <typename URBG> + static constexpr mock_call_fn MockCallFn() { + if constexpr (RandomMockingAccess::HasInvokeMock<URBG>::value) { + return MockCall<URBG>; + } else { + return nullptr; + } + } + + bool InvokeMock(FastTypeIdType key_id, void* args_tuple, void* result) { + if (mock_call_ == nullptr) return false; // avoids an indirect call. return mock_call_(t_erased_gen_ptr_, key_id, args_tuple, result); } - uintptr_t t_erased_gen_ptr_; + void* t_erased_gen_ptr_; mock_call_fn mock_call_; impl_fn generate_impl_fn_;
diff --git a/absl/random/bit_gen_ref_test.cc b/absl/random/bit_gen_ref_test.cc index ae5c1db..9c2ca0e 100644 --- a/absl/random/bit_gen_ref_test.cc +++ b/absl/random/bit_gen_ref_test.cc
@@ -114,13 +114,13 @@ // Implicit conversions allow passing MinStdRand to functions taking // absl::BitGenRef as well as explicitly constructing an absl::BitGenRef from // a MinStdRand. - operator absl::BitGenRef() const { + operator absl::BitGenRef() { conversion_count++; return absl::BitGenRef(minstd_gen); } std::minstd_rand minstd_gen; - mutable int conversion_count = 0; + int conversion_count = 0; }; TEST(BitGenRefTest, IsConvertibleTest) { @@ -145,6 +145,44 @@ } } +// Generating a variate mutates the generator, so binding to a const generator +// is disallowed. +static_assert(std::is_constructible_v<absl::BitGenRef, absl::BitGen&>); +static_assert(std::is_constructible_v<absl::BitGenRef, absl::BitGen&&>); +static_assert(!std::is_constructible_v<absl::BitGenRef, const absl::BitGen&>); +static_assert(!std::is_constructible_v<absl::BitGenRef, const absl::BitGen&&>); +static_assert(!std::is_constructible_v<absl::BitGenRef, const std::mt19937&>); + +// A URBG whose conversion operator is lvalue-only, which prevents binding an +// absl::BitGenRef to a temporary. +struct LvalueOnlyConversion { + using result_type = absl::BitGen::result_type; + static constexpr result_type(min)() { return (absl::BitGen::min)(); } + static constexpr result_type(max)() { return (absl::BitGen::max)(); } + result_type operator()() { return 0; } + + operator absl::BitGenRef() & { + conversion_count++; + return absl::BitGenRef(minstd_gen); + } + + std::minstd_rand minstd_gen; + int conversion_count = 0; +}; + +static_assert(std::is_convertible_v<LvalueOnlyConversion&, absl::BitGenRef>); +static_assert(!std::is_convertible_v<LvalueOnlyConversion, absl::BitGenRef>); + +TEST(BitGenRefTest, LvalueOnlyConversionIsUsed) { + // The conversion operator, rather than the URBG interface, is used. + LvalueOnlyConversion gen; + absl::BitGenRef gen_ref(gen); + EXPECT_EQ(gen.conversion_count, 1); + FnTest(gen); + EXPECT_EQ(gen.conversion_count, 2); + (void)gen_ref; +} + } // namespace ABSL_NAMESPACE_END } // namespace absl