Skip to content
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

Add missing deduction guides and use type_identity_t for allocator extended copy/move ctors #556

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+137 −10
Open

Add missing deduction guides and use type_identity_t for allocator extended copy/move ctors #556

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
93ddae5
Add additional type deduction guides for indirect
jbcoe Feb 16, 2025
4bf1917
Add deduction guides for polymorphic
jbcoe Feb 16, 2025
2197563
Add XYZ_HAS guards around deduction guide tests
jbcoe Feb 16, 2025
389687b
Disable type-deduction tests for all but C++20
jbcoe Feb 16, 2025
3bc0f8b
Undo edit to settings.json
jbcoe Feb 16, 2025
39b2654
Require a define to enable extended type deduction guides
jbcoe Feb 17, 2025
5f46074
Merge branch 'dev-jbcoe-deduction-guides' of github.com:jbcoe/value_t…
jbcoe Feb 17, 2025
af5477f
Merge branch 'main' into dev-jbcoe-deduction-guides
jbcoe Feb 21, 2025
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
2 changes: 2 additions & 0 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ cc_library(
srcs = ["indirect.cc"],
hdrs = ["indirect.h"],
copts = ["-Iexternal/value_types/"],
defines = ["XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION? Does it cause problems for some compilers?
I would expect that all compilers should be able to support these deduction guides, and so you wouldn't need to hide them under a build flag.

Copy link
Owner Author

@jbcoe jbcoe Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep extended features as explicit opt-in for now.

We check XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION in tests to ensure that appropriate tests are only run when extended features are enabled.

visibility = ["//visibility:public"],
deps = ["feature_check"],
)
Expand Down Expand Up @@ -91,6 +92,7 @@ cc_library(
srcs = ["polymorphic.cc"],
hdrs = ["polymorphic.h"],
copts = ["-Iexternal/value_types/"],
defines = ["XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION"],
visibility = ["//visibility:public"],
)

Expand Down
2 changes: 2 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ target_sources(xyz_value_types
xyz_add_library(
NAME indirect
ALIAS xyz_value_types::indirect
DEFINITIONS XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
)
target_sources(indirect
INTERFACE
Expand Down Expand Up @@ -75,6 +76,7 @@ target_sources(indirect_cxx17
xyz_add_library(
NAME polymorphic
ALIAS xyz_value_types::polymorphic
DEFINITIONS XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
)
target_sources(polymorphic
INTERFACE
Expand Down
14 changes: 14 additions & 0 deletions feature_check.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,18 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
#endif //(__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >=
// 202002L)

//
// XYZ_HAS_STD_TYPE_IDENTITY
// The macro is defined when std::type_identity_t<T> is available.
//

#ifdef XYZ_HAS_STD_TYPE_IDENTITY
#error "XYZ_HAS_STD_TYPE_IDENTITY is already defined"
#endif // XYZ_HAS_STD_TYPE_IDENTITY

#if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L)
#define XYZ_HAS_STD_TYPE_IDENTITY
#endif //(__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >=
// 202002L)
Comment on lines +98 to +110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro is set but never used. You use type_identity_t below without testing this macro.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in tests to disable tests that won't run on older C++ versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it's not used to guard those features in indirect.h. So for example this just errors out:

clang++ -O2 -std=c++17 -c indirect_test.cc

Theoretically you could support CTAD in C++17 (because CTAD itself was a C++17 feature); but right now you have only your C++14 version (which can't use CTAD) and your C++20 version (which assumes full C++20 support).

I'm figuring all this out as I write, so, forgive me if this is all old news. I guess the README already implies that C++17 isn't a supported target. You can probably at least partially ignore this comment, then.

I'm still confused why XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION is handled/set/tested in a different way from XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION, though.


#endif // XYZ_FEATURE_CHECK_H
13 changes: 10 additions & 3 deletions indirect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class indirect {
p_ = construct_from(alloc_, ilist, std::forward<Us>(us)...);
}

constexpr indirect(std::allocator_arg_t, const A& alloc,
constexpr indirect(std::allocator_arg_t, const std::type_identity_t<A>& alloc,
const indirect& other)
: alloc_(alloc) {
static_assert(std::copy_constructible<T>);
Expand All @@ -195,7 +195,7 @@ class indirect {
}

constexpr indirect(
std::allocator_arg_t, const A& alloc,
std::allocator_arg_t, const std::type_identity_t<A>& alloc,
indirect&& other) noexcept(allocator_traits::is_always_equal::value)
: p_(nullptr), alloc_(alloc) {
static_assert(std::move_constructible<T>);
Expand Down Expand Up @@ -465,10 +465,17 @@ concept is_hashable = requires(T t) { std::hash<T>{}(t); };
template <typename Value>
indirect(Value) -> indirect<Value>;

template <typename Alloc, typename Value>
template <typename Alloc, typename Value,
typename std::enable_if_t<!is_indirect_v<Value>, int> = 0>
Comment on lines +468 to +469
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <typename Alloc, typename Value,
typename std::enable_if_t<!is_indirect_v<Value>, int> = 0>
template <typename Alloc, typename Value>

The enable_if isn't needed, since the deduction guide on line 475 is more specific. (Please correct me if I'm wrong; I didn't test this.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken another look and the enable_if is needed for AppleClang at least.

This issue is possibly related: llvm/llvm-project#57646

Perhaps @Ukilele can comment as he'd had some success implementing deduction guides.

indirect(std::allocator_arg_t, Alloc, Value) -> indirect<
Value, typename std::allocator_traits<Alloc>::template rebind_alloc<Value>>;

#ifdef XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
template <typename Alloc, typename Value>
indirect(std::allocator_arg_t, std::type_identity_t<Alloc>,
indirect<Value, Alloc>) -> indirect<Value, Alloc>;
#endif // XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION

} // namespace xyz

template <class T, class Alloc>
Expand Down
40 changes: 36 additions & 4 deletions indirect_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,46 @@ TEST(IndirectTest, AllocatorExtendedInitializerListConstructor) {

#ifdef XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION
TEST(IndirectTest, TemplateArgumentDeduction) {
xyz::indirect a(42);
EXPECT_EQ(*a, 42);
xyz::indirect i(42);
EXPECT_EQ(*i, 42);
}

TEST(IndirectTest, TemplateArgumentDeductionCopy) {
xyz::indirect i(42);
xyz::indirect ii(i);

static_assert(std::is_same_v<decltype(i), decltype(ii)>);
EXPECT_EQ(*ii, 42);
}

TEST(IndirectTest, TemplateArgumentDeductionWithAllocator) {
xyz::indirect a(std::allocator_arg, std::allocator<int>{}, 42);
EXPECT_EQ(*a, 42);
xyz::indirect i(std::allocator_arg, xyz::TaggedAllocator<void>(1), 42);

static_assert(
std::is_same_v<decltype(i.get_allocator()), xyz::TaggedAllocator<int>>);
EXPECT_EQ(*i, 42);
}

#ifdef XYZ_HAS_STD_TYPE_IDENTITY
#ifdef XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
TEST(IndirectTest, TemplateArgumentDeductionWithDeducedAllocatorAndCopy) {
xyz::indirect i(std::allocator_arg, xyz::TaggedAllocator<int>(1), 42);
xyz::indirect ii(std::allocator_arg, 2, i);

static_assert(std::is_same_v<decltype(i.get_allocator()),
decltype(ii.get_allocator())>);
EXPECT_EQ(*ii, 42);
EXPECT_EQ(ii.get_allocator().tag, 2);
}

TEST(IndirectTest, TemplateArgumentDeductionWithDeducedAllocatorAndMove) {
xyz::indirect i(std::allocator_arg, xyz::TaggedAllocator<int>(1), 42);
xyz::indirect ii(std::allocator_arg, 2, std::move(i));
EXPECT_EQ(*ii, 42);
EXPECT_EQ(ii.get_allocator().tag, 2);
}
#endif // XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
#endif // XYZ_HAS_STD_TYPE_IDENTITY
#endif // XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION

template <typename Allocator = std::allocator<void>>
Expand Down
28 changes: 25 additions & 3 deletions polymorphic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ class direct_control_block final : public control_block<T, A> {

} // namespace detail

template <class T, class A>
class polymorphic;

template <class>
inline constexpr bool is_polymorphic_v = false;

template <class T, class A>
inline constexpr bool is_polymorphic_v<polymorphic<T, A>> = true;

template <class T, class A = std::allocator<T>>
class polymorphic {
using cblock_t = detail::control_block<T, A>;
Expand Down Expand Up @@ -241,7 +250,8 @@ class polymorphic {
cb_ = create_control_block<U>(ilist, std::forward<Ts>(ts)...);
}

constexpr polymorphic(std::allocator_arg_t, const A& alloc,
constexpr polymorphic(std::allocator_arg_t,
const std::type_identity_t<A>& alloc,
const polymorphic& other)
: alloc_(alloc) {
if (!other.valueless_after_move()) {
Expand All @@ -252,7 +262,7 @@ class polymorphic {
}

constexpr polymorphic(
std::allocator_arg_t, const A& alloc,
std::allocator_arg_t, const std::type_identity_t<A>& alloc,
polymorphic&& other) noexcept(allocator_traits::is_always_equal::value)
: alloc_(alloc) {
if constexpr (allocator_traits::is_always_equal::value) {
Expand Down Expand Up @@ -404,7 +414,19 @@ class polymorphic {
}
}
};

#ifdef XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
template <typename Value>
polymorphic(Value) -> polymorphic<Value>;

template <typename Alloc, typename Value,
typename std::enable_if_t<!is_polymorphic_v<Value>, int> = 0>
Comment on lines +421 to +422
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <typename Alloc, typename Value,
typename std::enable_if_t<!is_polymorphic_v<Value>, int> = 0>
template <typename Alloc, typename Value>

Same here: the deduction guide on line 427 is more specific so the enable_if isn't needed.

And a big bonus here: you can get rid of your xyz::is_polymorphic_v which is confusingly dissimilar to std::is_polymorphic_v.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::is_polymorphic_v<std::polymorphic<T>> evaluating to false is a pretty compelling call for a rename. A new issue shall be raised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, std::is_array_v<std::array<int, 10>> and std::is_function_v<std::function<int()>> are both false too. (My C++ Pub Quiz research is finally paying off! ;)) I think it's a bonus to be able to eliminate xyz::is_polymorphic, but if it's really indispensable, then I don't think you should worry about changing the name.

polymorphic(std::allocator_arg_t, Alloc, Value) -> polymorphic<
Value, typename std::allocator_traits<Alloc>::template rebind_alloc<Value>>;

template <typename Alloc, typename Value>
polymorphic(std::allocator_arg_t, std::type_identity_t<Alloc>,
polymorphic<Value, Alloc>) -> polymorphic<Value, Alloc>;
#endif // XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
} // namespace xyz

#endif // XYZ_POLYMORPHIC_H_
48 changes: 48 additions & 0 deletions polymorphic_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,4 +845,52 @@ TEST(PolymorphicTest, TaggedAllocatorsNotEqualMoveConstructFromValueless) {
EXPECT_TRUE(iii.valueless_after_move());
}

#ifdef XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION
#ifdef XYZ_HAS_STD_TYPE_IDENTITY
#ifdef XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
TEST(PolymorphicTest, TemplateArgumentDeduction) {
xyz::polymorphic p(Derived(4));

EXPECT_EQ(p->value(), 4);
}

TEST(PolymorphicTest, TemplateArgumentDeductionCopy) {
xyz::polymorphic p(Derived(4));
xyz::polymorphic pp(p);

static_assert(std::is_same_v<decltype(p), decltype(pp)>);

EXPECT_EQ(pp->value(), 4);
}

TEST(PolymorphicTest, TemplateArgumentDeductionWithAllocator) {
xyz::TaggedAllocator<int> a(1);
xyz::polymorphic p(std::allocator_arg, a, Derived(4));

EXPECT_EQ(p->value(), 4);
EXPECT_EQ(p.get_allocator().tag, 1);
}

TEST(PolymorphicTest, TemplateArgumentDeductionWithDeducedAllocatorAndCopy) {
xyz::TaggedAllocator<int> a(1);
xyz::polymorphic p(std::allocator_arg, a, Derived(4));
xyz::polymorphic pp(std::allocator_arg, 2, p);

EXPECT_EQ(pp->value(), 4);
EXPECT_EQ(pp.get_allocator().tag, 2);
}

TEST(PolymorphicTest, TemplateArgumentDeductionWithDeducedAllocatorAndMove) {
xyz::TaggedAllocator<int> a(1);
xyz::polymorphic p(std::allocator_arg, a, Derived(4));
xyz::polymorphic pp(std::allocator_arg, 2, std::move(p));

EXPECT_EQ(pp->value(), 4);
EXPECT_EQ(pp.get_allocator().tag, 2);
}

#endif // XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION
#endif // XYZ_HAS_STD_TYPE_IDENTITY
#endif // XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION

} // namespace