Skip to content

Commit b4d1be3

Browse files
cschreibcschreib
committed
Handle EOFT cases with no pre-allocated block
1 parent 8840ce9 commit b4d1be3

File tree

3 files changed

+166
-18
lines changed

3 files changed

+166
-18
lines changed

‎include/oup/observable_unique_ptr.hpp‎

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,8 @@ struct enable_observer_from_this_base {
299299
// Friendship is required for assignment of the observer.
300300
template<typename U, typename P, typename... Args>
301301
friend auto oup::make_observable(Args&&... args);
302+
template<typename U, typename D, typename P>
303+
friend class oup::basic_observable_ptr;
302304
};
303305
} // namespace details
304306

@@ -415,23 +417,34 @@ class basic_observable_ptr final {
415417
/**
416418
* \brief Decide whether to allocate a new control block or not.
417419
* \note If the object inherits from @ref basic_enable_observer_from_this, and
418-
* `Policy::is_sealed` is false (by construction this will always be the case when this
419-
* function is called), then we can just use the control block pointer stored in the
420-
* @ref basic_enable_observer_from_this base. Otherwise, we need to allocate a new one.
420+
* the base @ref basic_enable_observer_from_this is guaranteed to have a valid block
421+
* pointer, then we can reuse this. Otherwise, we may need to allocate a new one.
421422
*/
422423
template<typename U>
423-
control_block_type* get_block_from_object_(U* p) {
424+
control_block_type* get_or_create_block_from_object_(U* p) noexcept(
425+
queries::eoft_always_has_block() && has_enable_observer_from_this<U, Policy>) {
426+
424427
if (p == nullptr) {
425428
return nullptr;
426429
}
427430

428-
if constexpr (
429-
queries::eoft_constructor_allocates() && has_enable_observer_from_this<U, Policy>) {
430-
p->this_control_block->push_ref();
431-
return p->this_control_block;
431+
if constexpr (!has_enable_observer_from_this<U, Policy>) {
432+
return allocate_block_();
433+
} else {
434+
if constexpr (queries::eoft_always_has_block()) {
435+
p->this_control_block->push_ref();
436+
return p->this_control_block;
437+
} else {
438+
if (p->this_control_block != nullptr) {
439+
p->this_control_block->push_ref();
440+
return p->this_control_block;
441+
} else {
442+
control_block_type* new_block = allocate_block_();
443+
p->set_control_block_(new_block);
444+
return new_block;
445+
}
446+
}
432447
}
433-
434-
return allocate_block_();
435448
}
436449

437450
/**
@@ -571,7 +584,8 @@ class basic_observable_ptr final {
571584
typename U,
572585
typename enable =
573586
std::enable_if_t<std::is_convertible_v<U*, T*> && queries::owner_allow_release()>>
574-
explicit basic_observable_ptr(U* value) try :
587+
explicit basic_observable_ptr(U* value) noexcept(
588+
queries::eoft_always_has_block() && has_enable_observer_from_this<U, Policy>) try :
575589
basic_observable_ptr(get_or_create_block_from_object_(value), value) {
576590
} catch (...) {
577591
// Allocation of control block failed, delete input pointer and rethrow
@@ -592,7 +606,8 @@ class basic_observable_ptr final {
592606
typename U,
593607
typename enable =
594608
std::enable_if_t<std::is_convertible_v<U*, T*> && queries::owner_allow_release()>>
595-
explicit basic_observable_ptr(U* value, Deleter del) try :
609+
explicit basic_observable_ptr(U* value, Deleter del) noexcept(
610+
queries::eoft_always_has_block() && has_enable_observer_from_this<U, Policy>) try :
596611
basic_observable_ptr(get_or_create_block_from_object_(value), value, std::move(del)) {
597612
} catch (...) {
598613
// Allocation of control block failed, delete input pointer and rethrow
@@ -687,19 +702,26 @@ class basic_observable_ptr final {
687702
typename U,
688703
typename enable =
689704
std::enable_if_t<std::is_convertible_v<U*, T*> && queries::owner_allow_release()>>
690-
void reset(U* ptr) {
705+
void reset(U* ptr) noexcept(
706+
queries::eoft_always_has_block() && has_enable_observer_from_this<U, Policy>) {
691707
// Copy old pointer
692708
T* old_ptr = ptr_deleter.data;
693709
control_block_type* old_block = block;
694710

695711
// Assign the new one
696-
try {
712+
if constexpr (noexcept(get_or_create_block_from_object_(ptr))) {
713+
// There is always a control block available for us, so this cannot fail
697714
block = get_or_create_block_from_object_(ptr);
698715
ptr_deleter.data = ptr;
699-
} catch (...) {
700-
// Allocation of control block failed, delete input pointer and rethrow
701-
ptr_deleter(ptr);
702-
throw;
716+
} else {
717+
try {
718+
block = get_or_create_block_from_object_(ptr);
719+
ptr_deleter.data = ptr;
720+
} catch (...) {
721+
// Allocation of control block failed, delete input pointer and rethrow
722+
ptr_deleter(ptr);
723+
throw;
724+
}
703725
}
704726

705727
// Delete the old pointer

‎tests/runtime_tests.cpp‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3412,6 +3412,109 @@ TEST_CASE("observer from this non virtual unique", "[observer_from_this]") {
34123412
REQUIRE(mem_track.double_del() == 0u);
34133413
}
34143414

3415+
TEST_CASE("observer from this maybe no block unique", "[observer_from_this]") {
3416+
memory_tracker mem_track;
3417+
3418+
{
3419+
test_ptr_from_this_maybe_no_block ptr = oup::make_observable<
3420+
test_object_observer_from_this_maybe_no_block_unique, unique_maybe_no_block_policy>();
3421+
const test_ptr_from_this_maybe_no_block& cptr = ptr;
3422+
3423+
test_optr_from_this_maybe_no_block_unique optr_from_this = ptr->observer_from_this();
3424+
test_optr_from_this_const_maybe_no_block_unique optr_from_this_const =
3425+
cptr->observer_from_this();
3426+
3427+
REQUIRE(instances == 1);
3428+
REQUIRE(optr_from_this.expired() == false);
3429+
REQUIRE(optr_from_this_const.expired() == false);
3430+
REQUIRE(optr_from_this.get() == ptr.get());
3431+
REQUIRE(optr_from_this_const.get() == ptr.get());
3432+
}
3433+
3434+
REQUIRE(instances == 0);
3435+
REQUIRE(mem_track.leaks() == 0u);
3436+
REQUIRE(mem_track.double_del() == 0u);
3437+
}
3438+
3439+
TEST_CASE("observer from this maybe no block acquire", "[observer_from_this]") {
3440+
memory_tracker mem_track;
3441+
3442+
{
3443+
test_ptr_from_this_maybe_no_block ptr{
3444+
new test_object_observer_from_this_maybe_no_block_unique};
3445+
const test_ptr_from_this_maybe_no_block& cptr = ptr;
3446+
3447+
test_optr_from_this_maybe_no_block_unique optr_from_this = ptr->observer_from_this();
3448+
test_optr_from_this_const_maybe_no_block_unique optr_from_this_const =
3449+
cptr->observer_from_this();
3450+
3451+
REQUIRE(instances == 1);
3452+
REQUIRE(optr_from_this.expired() == false);
3453+
REQUIRE(optr_from_this_const.expired() == false);
3454+
REQUIRE(optr_from_this.get() == ptr.get());
3455+
REQUIRE(optr_from_this_const.get() == ptr.get());
3456+
}
3457+
3458+
REQUIRE(instances == 0);
3459+
REQUIRE(mem_track.leaks() == 0u);
3460+
REQUIRE(mem_track.double_del() == 0u);
3461+
}
3462+
3463+
TEST_CASE("observer from this maybe no block reset to new", "[observer_from_this]") {
3464+
memory_tracker mem_track;
3465+
3466+
{
3467+
auto* raw_ptr1 = new test_object_observer_from_this_maybe_no_block_unique;
3468+
auto* raw_ptr2 = new test_object_observer_from_this_maybe_no_block_unique;
3469+
3470+
test_ptr_from_this_maybe_no_block ptr{raw_ptr1};
3471+
const test_ptr_from_this_maybe_no_block& cptr = ptr;
3472+
3473+
ptr.reset(raw_ptr2);
3474+
3475+
test_optr_from_this_maybe_no_block_unique optr_from_this = ptr->observer_from_this();
3476+
test_optr_from_this_const_maybe_no_block_unique optr_from_this_const =
3477+
cptr->observer_from_this();
3478+
3479+
REQUIRE(instances == 1);
3480+
REQUIRE(optr_from_this.expired() == false);
3481+
REQUIRE(optr_from_this_const.expired() == false);
3482+
REQUIRE(optr_from_this.get() == ptr.get());
3483+
REQUIRE(optr_from_this.get() == raw_ptr2);
3484+
REQUIRE(optr_from_this_const.get() == ptr.get());
3485+
REQUIRE(optr_from_this_const.get() == raw_ptr2);
3486+
}
3487+
3488+
REQUIRE(instances == 0);
3489+
REQUIRE(mem_track.leaks() == 0u);
3490+
REQUIRE(mem_track.double_del() == 0u);
3491+
}
3492+
3493+
TEST_CASE("observer from this maybe no block reset to new bad alloc", "[observer_from_this]") {
3494+
memory_tracker mem_track;
3495+
3496+
{
3497+
auto* raw_ptr1 = new test_object_observer_from_this_maybe_no_block_unique;
3498+
auto* raw_ptr2 = new test_object_observer_from_this_maybe_no_block_unique;
3499+
3500+
test_ptr_from_this_maybe_no_block ptr{raw_ptr1};
3501+
3502+
try {
3503+
force_allocation_failure = true;
3504+
ptr.reset(raw_ptr2);
3505+
} catch (...) {
3506+
}
3507+
3508+
force_allocation_failure = false;
3509+
REQUIRE(instances == 1);
3510+
REQUIRE(ptr.get() == raw_ptr1);
3511+
}
3512+
3513+
REQUIRE(instances == 0);
3514+
REQUIRE(mem_track.leaks() == 0u);
3515+
REQUIRE(mem_track.double_del() == 0u);
3516+
}
3517+
34153518
TEST_CASE("observer from this virtual sealed", "[observer_from_this]") {
34163519
memory_tracker mem_track;
34173520

‎tests/tests_common.hpp‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,14 @@ struct unique_non_virtual_policy {
8181
using observer_policy = oup::default_observer_policy;
8282
};
8383

84+
struct unique_maybe_no_block_policy {
85+
static constexpr bool is_sealed = false;
86+
static constexpr bool allow_eoft_in_constructor = false;
87+
static constexpr bool allow_eoft_multiple_inheritance = true;
88+
static constexpr bool eoft_constructor_takes_control_block = false;
89+
using observer_policy = oup::default_observer_policy;
90+
};
91+
8492
struct test_object_observer_from_this_virtual_sealed :
8593
public test_object,
8694
public oup::basic_enable_observer_from_this<
@@ -99,6 +107,12 @@ struct test_object_observer_from_this_non_virtual_unique :
99107
unique_non_virtual_policy>(block) {}
100108
};
101109

110+
struct test_object_observer_from_this_maybe_no_block_unique :
111+
public test_object,
112+
public oup::basic_enable_observer_from_this<
113+
test_object_observer_from_this_maybe_no_block_unique,
114+
unique_maybe_no_block_policy> {};
115+
102116
struct test_object_thrower_observer_from_this_non_virtual_unique :
103117
public oup::basic_enable_observer_from_this<
104118
test_object_thrower_observer_from_this_non_virtual_unique,
@@ -275,6 +289,11 @@ using test_ptr_from_this_non_virtual = oup::basic_observable_ptr<
275289
oup::default_delete,
276290
unique_non_virtual_policy>;
277291

292+
using test_ptr_from_this_maybe_no_block = oup::basic_observable_ptr<
293+
test_object_observer_from_this_maybe_no_block_unique,
294+
oup::default_delete,
295+
unique_maybe_no_block_policy>;
296+
278297
using test_sptr_from_this_virtual = oup::basic_observable_ptr<
279298
test_object_observer_from_this_virtual_sealed,
280299
oup::placement_delete,
@@ -323,6 +342,10 @@ using test_optr_from_this_non_virtual_unique =
323342
oup::observer_ptr<test_object_observer_from_this_non_virtual_unique>;
324343
using test_optr_from_this_const_non_virtual_unique =
325344
oup::observer_ptr<const test_object_observer_from_this_non_virtual_unique>;
345+
using test_optr_from_this_maybe_no_block_unique =
346+
oup::observer_ptr<test_object_observer_from_this_maybe_no_block_unique>;
347+
using test_optr_from_this_const_maybe_no_block_unique =
348+
oup::observer_ptr<const test_object_observer_from_this_maybe_no_block_unique>;
326349
using test_optr_from_this_virtual_sealed =
327350
oup::observer_ptr<test_object_observer_from_this_virtual_sealed>;
328351
using test_optr_from_this_const_virtual_sealed =

0 commit comments

Comments
 (0)