Skip to content

Commit

Permalink
In {,Intrusive}SharedPtr<T>, use plain assignment instead of
Browse files Browse the repository at this point in the history
`riegeli::Reset()` for assigning `T` from `T&&`.

`riegeli::Reset()` is designed for using methods which are equivalent to
assigning an object constructed from other constructor arguments. If the
constructor argument is just `T&&`, there is nothing to construct before the
assignment, and the idiomatic way to customize this is assignment, not
`Reset()`.

Also, simplify `IntrusiveSharedPtr::ResetImpl()` by introducing a helper
predicate, like in `SharedPtr::ResetImpl()`.

`riegeli::Reset()` continues to be used for assigning `T` from `Initializer<T>`.
This dispatches to methods which use other `Reset()` variants depending on how
the `Initializer<T>` was constructed, e.g. for `Initializer<T>` constructed
from `MakerType<Args..>` this calls `Reset(T&, Args...)`, which can be more
efficient.

Sadly, the constraint for usability of `riegeli::Reset(T&, Initializer<T>)`
must continue to be `std::move_assignable<T>`, because even if e.g.
`MakerType<Args...>` supported `riegeli::Reset()` under weaker constraints, type
erasure in `Initializer<T>` requires all ways of constructing `Initializer<T>`
to support `riegeli::Reset()`. In particular for `Initializer<T>` constructed
from `T&&` this requires `std::move_assignable<T>`.

There not enough usages of `riegeli::Reset()` directly with `MakerType`
or `MakerTypeFor` (not through `Initializer`) to warrant weakening their
constraints (which could be done by introducing a `SupportsReset` predicate
and using it for them, instead of the current `std::constructible<T, Args...>`
and `std::is_move_assignable<T>).

In summary, `riegeli::Reset()` should be viewed as an optimization of assigning
a constructed object, not a generalization to support new ways of replacing the
state of non-move-assignable types.

PiperOrigin-RevId: 692199724
  • Loading branch information
QrczakMK committed Nov 2, 2024
1 parent 686019a commit 71a0ab7
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 33 deletions.
1 change: 0 additions & 1 deletion riegeli/base/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,6 @@ cc_library(
":external_ref_support",
":initializer",
":new_aligned",
":reset",
"@com_google_absl//absl/base:core_headers",
"@com_google_absl//absl/base:nullability",
"@com_google_absl//absl/meta:type_traits",
Expand Down
36 changes: 14 additions & 22 deletions riegeli/base/intrusive_shared_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include "riegeli/base/external_ref_support.h" // IWYU pragma: keep
#include "riegeli/base/initializer.h"
#include "riegeli/base/ownership.h"
#include "riegeli/base/reset.h"

namespace riegeli {

Expand Down Expand Up @@ -334,33 +333,26 @@ class
if (ptr != nullptr) ptr->Unref();
}

template <
typename DependentT = T,
std::enable_if_t<
absl::conjunction<
intrusive_shared_ptr_internal::HasHasUniqueOwner<DependentT>,
absl::disjunction<
absl::negation<std::has_virtual_destructor<DependentT>>,
std::is_final<DependentT>>,
std::is_move_assignable<DependentT>>::value,
int> = 0>
template <typename DependentT>
struct IsAssignable
: absl::conjunction<
intrusive_shared_ptr_internal::HasHasUniqueOwner<DependentT>,
absl::disjunction<
absl::negation<std::has_virtual_destructor<DependentT>>,
std::is_final<DependentT>>,
std::is_move_assignable<DependentT>> {};

template <typename DependentT = T,
std::enable_if_t<IsAssignable<DependentT>::value, int> = 0>
void ResetImpl(Initializer<T> value) {
if (IsUnique()) {
riegeli::Reset(*ptr_, std::move(value));
*ptr_ = std::move(value);
return;
}
Unref(std::exchange(ptr_, std::move(value).MakeUnique().release()));
}
template <
typename DependentT = T,
std::enable_if_t<
absl::disjunction<
absl::negation<
intrusive_shared_ptr_internal::HasHasUniqueOwner<DependentT>>,
absl::conjunction<std::has_virtual_destructor<DependentT>,
absl::negation<std::is_final<DependentT>>>,
absl::negation<std::is_move_assignable<DependentT>>>::value,
int> = 0>
template <typename DependentT = T,
std::enable_if_t<!IsAssignable<DependentT>::value, int> = 0>
void ResetImpl(Initializer<T> value) {
Unref(std::exchange(ptr_, std::move(value).MakeUnique().release()));
}
Expand Down
12 changes: 4 additions & 8 deletions riegeli/base/maker.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ class MakerType : public ConditionallyAssignable<absl::conjunction<
template <
typename T,
std::enable_if_t<absl::conjunction<absl::negation<std::is_reference<T>>,
std::is_constructible<T, Args&&...>,
std::is_move_assignable<T>>::value,
SupportsReset<T, Args&&...>>::value,
int> = 0>
friend void RiegeliReset(T& dest, MakerType&& src) {
absl::apply(
Expand All @@ -173,8 +172,7 @@ class MakerType : public ConditionallyAssignable<absl::conjunction<
template <typename T,
std::enable_if_t<
absl::conjunction<absl::negation<std::is_reference<T>>,
std::is_constructible<T, const Args&...>,
std::is_move_assignable<T>>::value,
SupportsReset<T, const Args&...>>::value,
int> = 0>
friend void RiegeliReset(T& dest, const MakerType& src) {
absl::apply([&](const Args&... args) { riegeli::Reset(dest, args...); },
Expand Down Expand Up @@ -339,8 +337,7 @@ class MakerTypeFor : public ConditionallyAssignable<absl::conjunction<
template <typename DependentT = T,
std::enable_if_t<
absl::conjunction<absl::negation<std::is_reference<DependentT>>,
std::is_constructible<DependentT, Args&&...>,
std::is_move_assignable<DependentT>>::value,
SupportsReset<DependentT, Args&&...>>::value,
int> = 0>
friend void RiegeliReset(T& dest, MakerTypeFor&& src) {
riegeli::Reset(dest, std::move(src).maker());
Expand All @@ -349,8 +346,7 @@ class MakerTypeFor : public ConditionallyAssignable<absl::conjunction<
typename DependentT = T,
std::enable_if_t<
absl::conjunction<absl::negation<std::is_reference<DependentT>>,
std::is_constructible<DependentT, const Args&...>,
std::is_move_assignable<DependentT>>::value,
SupportsReset<DependentT, const Args&...>>::value,
int> = 0>
friend void RiegeliReset(T& dest, const MakerTypeFor& src) {
riegeli::Reset(dest, src.maker());
Expand Down
10 changes: 10 additions & 0 deletions riegeli/base/reset.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,16 @@ struct HasAssignment : HasAssignmentImpl<void, T, Args...> {};

} // namespace reset_internal

// `SupportsReset<T, Args...>::value` is true if `riegeli::Reset(T&, Args...)`
// is supported.
template <typename T, typename... Args>
struct SupportsReset
: absl::disjunction<reset_internal::HasRiegeliReset<T, Args...>,
reset_internal::HasReset<T, Args...>,
reset_internal::HasAssignment<T, Args...>,
absl::conjunction<std::is_constructible<T, Args...>,
std::is_move_assignable<T>>> {};

template <typename T, typename... Args,
std::enable_if_t<reset_internal::HasRiegeliReset<T, Args...>::value,
int> = 0>
Expand Down
3 changes: 1 addition & 2 deletions riegeli/base/shared_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include "riegeli/base/initializer.h"
#include "riegeli/base/new_aligned.h"
#include "riegeli/base/ref_count.h"
#include "riegeli/base/reset.h"

namespace riegeli {

Expand Down Expand Up @@ -413,7 +412,7 @@ class
std::enable_if_t<IsAssignable<DependentT>::value, int> = 0>
void ResetImpl(Initializer<T> value) {
if (IsUnique()) {
riegeli::Reset(*ptr_, std::move(value));
*ptr_ = std::move(value);
return;
}
Unref(std::exchange(ptr_, New(std::move(value))));
Expand Down

0 comments on commit 71a0ab7

Please sign in to comment.