From 5b2ac60991fe41d4d261b4a3184aeac0b5d3e5aa Mon Sep 17 00:00:00 2001 From: Marcin Kowalczyk Date: Wed, 16 Oct 2024 23:31:47 +0200 Subject: [PATCH] Do not bother with the `CanBindTo` optimization in `MakerType{,For}`. The optimization is important in `Initializer` constructed from an already constructed object, but it is rare to explicitly wrap constructor arguments in a way can benefit from copy elision. `MakerType{,For}::Reference()` is still useful to support `Initializer::Reference()`, and is compatible with immovable types before C++17 which guarantees copy elision. If copy elision is guaranteed and `storage` is not provided, `MakerType{,For}::Reference()` returns the result by value instead of by reference, which is a more efficient way to construct the temporary. Rename `internal::CanBindTo` to `internal::CanBindReference`, and limit it to a single argument case which remains used. Add `internal::BindReference()` function template, and use it to implicitly convert references instead of applying the pointer hack where `CanBindReference` is used (needed because not all compilers implement http://wg21.link/cwg2352). PiperOrigin-RevId: 686644549 --- riegeli/base/initializer.h | 17 +-- riegeli/base/initializer_internal.h | 40 +++++-- riegeli/base/maker.h | 166 +++++++++++----------------- 3 files changed, 100 insertions(+), 123 deletions(-) diff --git a/riegeli/base/initializer.h b/riegeli/base/initializer.h index ed83453d..eaf81dcc 100644 --- a/riegeli/base/initializer.h +++ b/riegeli/base/initializer.h @@ -194,8 +194,9 @@ class InitializerBase { #endif } - // Constructs the `T`, or returns a reference to an already constructed object - // if that was passed to the `Initializer`. + // Constructs the `T` in `storage` which must outlive the returned reference, + // or returns a reference to an already constructed object if a compatible + // object was passed to `Initializer` constructor. // // `Reference()` instead of conversion to `T` can avoid moving the object if // the caller does not need to store the object, or if it will be moved later @@ -237,11 +238,11 @@ class InitializerBase { TemporaryStorage&& storage); template ::value, int> = 0> + std::enable_if_t::value, int> = 0> static T&& ReferenceMethodFromObject(TypeErasedRef context, TemporaryStorage&& storage); template ::value, int> = 0> + std::enable_if_t::value, int> = 0> static T&& ReferenceMethodFromObject(TypeErasedRef context, TemporaryStorage&& storage); @@ -921,15 +922,17 @@ T&& InitializerBase::ReferenceMethodDefault( } template -template ::value, int>> +template ::value, int>> T&& InitializerBase::ReferenceMethodFromObject( TypeErasedRef context, ABSL_ATTRIBUTE_UNUSED TemporaryStorage&& storage) { - return context.Cast(); + return BindReference(context.Cast()); } template -template ::value, int>> +template ::value, int>> T&& InitializerBase::ReferenceMethodFromObject( TypeErasedRef context, TemporaryStorage&& storage) { return std::move(storage).emplace(context.Cast()); diff --git a/riegeli/base/initializer_internal.h b/riegeli/base/initializer_internal.h index c7fe2b56..f6e10726 100644 --- a/riegeli/base/initializer_internal.h +++ b/riegeli/base/initializer_internal.h @@ -17,35 +17,51 @@ #include +#include "absl/base/casts.h" + namespace riegeli { namespace initializer_internal { -// `CanBindTo::value` is `true` if constructing `T(args...)` -// with `args...` of type `Args&&...` can be elided, with `T&&` binding directly -// to the only element of `args...` instead. +// `CanBindReference::value` is `true` if `Arg&&` can be implicitly +// converted to `T&&` without creating a temporary. // // Due to not all compilers implementing http://wg21.link/cwg2352 (converting // `T*&` to `const T* const&` could have bound the result to a temporary), -// binding a const reference should be done by converting between the -// corresponding pointer types and then dereferencing. +// this covers also the case when the corresponding pointers can be converted. +// `BindReference()` should be used for the actual conversion. -template -struct CanBindTo : std::false_type {}; +template +struct CanBindReference : std::false_type {}; template -struct CanBindTo : std::is_convertible {}; +struct CanBindReference : std::is_convertible {}; template -struct CanBindTo : std::false_type {}; +struct CanBindReference : std::false_type {}; template -struct CanBindTo : std::is_convertible {}; +struct CanBindReference : std::is_convertible { +}; template -struct CanBindTo : std::false_type {}; +struct CanBindReference : std::false_type {}; template -struct CanBindTo : std::is_convertible {}; +struct CanBindReference : std::is_convertible {}; + +// `BindReference(arg)` returns `arg` implicitly converted to `T&&`. +// +// Due to not all compilers implementing http://wg21.link/cwg2352 (converting +// `T*&` to `const T* const&` could have bound the result to a temporary), +// this is not implemented as a simple implicit conversion, but by converting +// the reference to a pointer, implicitly converting the pointer, and +// dereferencing back. +template ::value, int> = 0> +inline T&& BindReference(Arg&& arg) { + return std::forward( + *absl::implicit_cast*>(&arg)); +} } // namespace initializer_internal } // namespace riegeli diff --git a/riegeli/base/maker.h b/riegeli/base/maker.h index 3ba1dd1c..7a02abcc 100644 --- a/riegeli/base/maker.h +++ b/riegeli/base/maker.h @@ -23,10 +23,8 @@ #include #include "absl/base/attributes.h" -#include "absl/base/casts.h" #include "absl/meta/type_traits.h" #include "absl/utility/utility.h" -#include "riegeli/base/initializer_internal.h" #include "riegeli/base/reset.h" #include "riegeli/base/temporary_storage.h" #include "riegeli/base/type_traits.h" @@ -97,77 +95,53 @@ class MakerType : public ConditionallyAssignable(ptr, std::index_sequence_for()); } - // Constructs the `T`, or returns a reference to an already constructed object - // if that was passed to the `MakerType`. + // Constructs the `T` in `storage` which must outlive the returned reference. // - // `Reference()` instead of `Construct()` can avoid moving the object if the - // caller does not need to store the object, or if it will be moved later - // because the target location for the object is not ready yet. + // `Reference()` instead of `Construct()` supports `Initializer::Reference()`, + // and is compatible with immovable types before C++17 which guarantees copy + // elision. // - // `storage` must outlive usages of the returned reference. + // If copy elision is guaranteed and the `storage` argument is omitted, the + // result is returned by value instead of by reference, which is a more + // efficient way to construct the temporary. +#if __cpp_guaranteed_copy_elision template < typename T, - std::enable_if_t, - initializer_internal::CanBindTo< - T&&, Args&&...>>::value, - int> = 0> - T&& Reference() && ABSL_ATTRIBUTE_LIFETIME_BOUND { - auto&& reference = std::get<0>(std::move(args_)); - return std::forward( - *absl::implicit_cast*>(&reference)); + std::enable_if_t::value, int> = 0> + T Reference() && { + return std::move(*this).template Construct(); } +#endif template < typename T, - std::enable_if_t, - initializer_internal::CanBindTo< - T&&, Args&&...>>::value, - int> = 0> - T&& Reference(ABSL_ATTRIBUTE_UNUSED TemporaryStorage&& storage) && - ABSL_ATTRIBUTE_LIFETIME_BOUND { - return std::move(*this).template Reference(); - } - template , - absl::negation>>::value, - int> = 0> - T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND = - TemporaryStorage()) && { + std::enable_if_t::value, int> = 0> + T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND +#if !__cpp_guaranteed_copy_elision + = TemporaryStorage() +#endif + ) && { return absl::apply( [&](Args&&... args) -> T&& { return std::move(storage).emplace(std::forward(args)...); }, std::move(args_)); } +#if __cpp_guaranteed_copy_elision template , - initializer_internal::CanBindTo< - T&&, const Args&...>>::value, - int> = 0> - T&& Reference() const& ABSL_ATTRIBUTE_LIFETIME_BOUND { - return std::forward( - *absl::implicit_cast*>(&std::get<0>(args_))); - } - template , - initializer_internal::CanBindTo< - T&&, const Args&...>>::value, - int> = 0> - T&& Reference(ABSL_ATTRIBUTE_UNUSED TemporaryStorage&& storage) - const& ABSL_ATTRIBUTE_LIFETIME_BOUND { - return Reference(); + std::enable_if_t::value, + int> = 0> + T Reference() const& { + return Construct(); } +#endif template , - absl::negation>>::value, + std::enable_if_t::value, int> = 0> - T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND = - TemporaryStorage()) const& { + T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND +#if !__cpp_guaranteed_copy_elision + = TemporaryStorage() +#endif + ) const& { return absl::apply( [&](const Args&... args) -> T&& { return std::move(storage).emplace(args...); @@ -261,35 +235,23 @@ class MakerTypeForBase std::move(*this).maker().template ConstructAt(ptr); } - // Constructs the `T`, or returns a reference to an already constructed object - // if that was passed to the `MakerTypeFor`. + // Constructs the `T` in `storage` which must outlive the returned reference. // - // `Reference()` instead of conversion to `T` can avoid moving the object if - // the caller does not need to store the object, or if it will be moved later - // because the target location for the object is not ready yet. + // `Reference()` instead of conversion to `T` supports + // `Initializer::Reference()`, and is compatible with immovable types before + // C++17 which guarantees copy elision. // - // `storage` must outlive usages of the returned reference. - template ::value, - int> = 0> - T&& Reference() && ABSL_ATTRIBUTE_LIFETIME_BOUND { - return std::move(*this).maker().template Reference(); - } - template ::value, - int> = 0> - T&& Reference(ABSL_ATTRIBUTE_UNUSED TemporaryStorage&& storage) && - ABSL_ATTRIBUTE_LIFETIME_BOUND { - return std::move(*this).Reference(); - } - template ::value, - int> = 0> - T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND = - TemporaryStorage()) && { + // If copy elision is guaranteed and the `storage` argument is omitted, the + // result is returned by value instead of by reference, which is a more + // efficient way to construct the temporary. +#if __cpp_guaranteed_copy_elision + T Reference() && { return std::move(*this).maker().template Reference(); } +#endif + T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND +#if !__cpp_guaranteed_copy_elision + = TemporaryStorage() +#endif + ) && { return std::move(*this).maker().template Reference(std::move(storage)); } @@ -357,28 +319,24 @@ class MakerTypeForByConstBase this->maker().template ConstructAt(ptr); } + // Constructs the `T` in `storage` which must outlive the returned reference. + // + // `Reference()` instead of conversion to `T` supports + // `Initializer::Reference()`, and is compatible with immovable types before + // C++17 which guarantees copy elision. + // + // If copy elision is guaranteed and the `storage` argument is omitted, the + // result is returned by value instead of by reference, which is a more + // efficient way to construct the temporary. using MakerTypeForByConstBase::MakerTypeForBase::Reference; - template ::value, - int> = 0> - T&& Reference() const& ABSL_ATTRIBUTE_LIFETIME_BOUND { - return this->maker().template Reference(); - } - template ::value, - int> = 0> - T&& Reference(ABSL_ATTRIBUTE_UNUSED TemporaryStorage&& storage) - const& ABSL_ATTRIBUTE_LIFETIME_BOUND { - return Reference(); - } - template ::value, - int> = 0> - T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND = - TemporaryStorage()) const& { +#if __cpp_guaranteed_copy_elision + T Reference() const& { return this->maker().template Reference(); } +#endif + T&& Reference(TemporaryStorage&& storage ABSL_ATTRIBUTE_LIFETIME_BOUND +#if !__cpp_guaranteed_copy_elision + = TemporaryStorage() +#endif + ) const& { return this->maker().template Reference(std::move(storage)); }