Skip to content

Tighten absl::BitGenRef binding rules and simplify its implementation. #2294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
+83 −33
Draft

Tighten absl::BitGenRef binding rules and simplify its implementation. #2294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 43 additions & 31 deletions absl/random/bit_gen_ref.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <cstdint>
#include <limits>
#include <memory>
#include <type_traits>

#include "absl/base/attributes.h"
Expand Down Expand Up @@ -69,13 +70,25 @@ class BitGenRef {
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;
Expand All @@ -84,27 +97,20 @@ class BitGenRef {

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;

Expand All @@ -119,35 +125,41 @@ class BitGenRef {
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 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);
}
static bool NotAMock(uintptr_t, FastTypeIdType, void*, void*) {
return false;

// 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;
}
}

inline bool InvokeMock(FastTypeIdType key_id, void* args_tuple,
void* result) {
if (mock_call_ == NotAMock) return false; // avoids an indirect call.
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_;

Expand Down
42 changes: 40 additions & 2 deletions absl/random/bit_gen_ref_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,13 @@ struct MinStdRand {
// 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) {
Expand All @@ -145,6 +145,44 @@ TEST(BitGenRefTest, IsConvertibleTest) {
}
}

// 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
Loading