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

Jump to bottom

weak_ptr: Make it possible to produce constant pointers #2226

Merged
merged 1 commit into from
May 16, 2024
Merged

weak_ptr: Make it possible to produce constant pointers #2226

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions include/seastar/core/weak_ptr.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ template<typename T>
class weak_ptr {
template<typename U>
friend class weakly_referencable;
template <typename U>
friend class weak_ptr;
private:
using hook_type = boost::intrusive::list_member_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>;
hook_type _hook;
Expand All @@ -59,7 +61,18 @@ private:
_hook.swap_nodes(o._hook);
std::swap(_ptr, o._ptr);
}

public:
template <typename U>
requires std::convertible_to<U*, T*>
weak_ptr(weak_ptr<U>&& o)
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic: it's nicer to specify this as weak_ptr(std::convertible_to<T&> auto&& o), as we don't introduce a variable U which is immediately forgotten (if it works).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't manage to make it work this-or-similar way.

In particular, this exact syntax fails like this

/home/xemul/src/seastar/tests/unit/weak_ptr_test.cc:66:24: note:
no known conversion for argument 1 from ‘weak_ptr<myiclass>’ to ‘weak_ptr<baseclass>’

I guess it's because std::convertible applies to pointer type, while o's type is about weak_ptr<> over the type itself, so compiler doesn't match one against each other

Copy link
Member

Choose a reason for hiding this comment

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

I thought it would know to convert references. But it's just cosmetic anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Is there anything else that prevents this patch from being merged? :)

{
if (o._ptr) {
_ptr = std::exchange(o._ptr, nullptr);
_hook.swap_nodes(o._hook);
}
}

// Note: The default constructor's body is implemented as no-op
// rather than `noexcept = default` due to a bug with gcc 9.3.1
// that deletes the constructor since boost::intrusive::list_member_hook
Expand Down Expand Up @@ -139,6 +152,10 @@ public:
_ptr_list.push_back(ptr);
return ptr;
}

weak_ptr<const T> weak_from_this() const noexcept {
return const_cast<weakly_referencable*>(this)->weak_from_this();
}
Copy link
Contributor

@tchaikov tchaikov May 8, 2024 •
edited

Choose a reason for hiding this comment

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

@xemul hi Pavel, i'd propose a different, but probably more standard compliant, and extensive solution to address this problem.

from the standard, weak_ptr<Y> should be constructible from another weak_ptr<T>. as long as Y is "compatible" with T. see https://eel.is/c++draft/util.sharedptr#util.smartptr.shared.general-6 .

so instead of marking _ptr_list mutable. we could just use something like:

namespace internal {

// quote from
// http://eel.is/c++draft/util.sharedptr#util.smartptr.shared.general-6,
//
// a pointer type Y* is said to be compatible with a pointer type T* when
// either Y* is convertible to T* or Y is U[N] and T is cv U[].
template <typename Y, typename T>
struct bounded_array_of : std::false_type {};

template <typename U, typename T, size_t N>
struct bounded_array_of<U[N], T> : std::is_same<std::remove_cv_t<T>, U[]> {};

template <typename Y, typename T>
concept pointer_type_compatible_with = (std::convertible_to<Y*, T*> ||
                                        bounded_array_of<Y, T>::value);
}
///...
template<typename T>
class weak_ptr {
    template<typename U>
    friend class weakly_referencable;
    template<typename U>
    friend class weak_ptr;
private:
///...
    template<internal::pointer_type_compatible_with<T> Y>
    weak_ptr(const weak_ptr<Y>& o) noexcept {
        if (o._ptr) {
            swap(o._ptr->weak_from_this());
        }
    }
///...
template<typename T>
class weakly_referencable {
///...
    weak_ptr<T> weak_from_this() const noexcept {
        return const_cast<weakly_referencable*>(this)->weak_from_this();
    }
};

for couple reasons:

  1. more standard compliant. so easier to learn from a regular C++ programmer's perspective
  2. we get a better weak_ptr, as casting to a compatible pointer type should be possible, and this behavior is more intuitive.
  3. less mutable. personally, i'd avoid it if possible. as it is an exception, which means.. surprise.

please note, instead of using std::is_bounded_array, i am creating yet another type trait here, as std::is_bounded_array does not provide convenient way to access the element type, and the proposed implementation is basically a straightforward translation of the standard. so i prefer (re)inventing it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mutable is not required, I erroneously forgot to remove it. But I like the idea of introducing weak_ptr<const T>(weak_ptr<T>&&) constructor, I'll try it.

However at this point no more flexibility is requires from my perspective. In particular, your suggestion means that weak_ptr<T>(weak_ptr<const T>&&) will be introduced, since `std::is_convertible<const T, T>::value == true, but it's not correct, casting const pointer to non-const shouldn't happen automatically

Copy link
Contributor

Choose a reason for hiding this comment

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

i think you misread the line of

std::convertible_to<Y*, T*>

also, my original intention was not "flexibility", but more about standard compliance and correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think you misread the line of

std::convertible_to<Y*, T*>

Yes, thank you, I misread the *'s :( However, I'm still concerned about the bounded_array. The

template <typename Y, typename T>
concept pointer_type_compatible_with = (std::convertible_to<Y*, T*> ||
                                        bounded_array_of<Y, T>::value);

means that we can create weak_ptr<T> from weak_ptr<Y> provided std::converible_to<Y, T>, but in case Y is an array, we cannot get weak_ptr<Y> itself, because array cannot inherit from weakly_referencable

Copy link
Contributor

@tchaikov tchaikov May 9, 2024 •
edited

Choose a reason for hiding this comment

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

sorry, i was appreciating the design of the standard library, and completely ignored the connection between seastar::weak_ptr and seastar::weakly_referencable . as in our implementation, we don't create a seastar::weak_ptr from an existing instance of seastar::shared_ptr or seastar::lw_shared_ptr, like the standard library does. so we have to to rely on the type of T instead of its smart pointer wrapper.

};

}
Expand Down
27 changes: 27 additions & 0 deletions tests/unit/weak_ptr_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,33 @@ BOOST_AUTO_TEST_CASE(test_weak_ptr_is_reset) {
BOOST_REQUIRE(!bool(wp));
}

BOOST_AUTO_TEST_CASE(test_const_weak_ptr) {
auto owning_ptr = std::make_unique<myclass>();

weak_ptr<const myclass> cwptr = const_cast<const myclass&>(*owning_ptr).weak_from_this();
nyh marked this conversation as resolved.
Show resolved Hide resolved
BOOST_REQUIRE(bool(cwptr));
owning_ptr.reset();
BOOST_REQUIRE(!bool(cwptr));
}

class baseclass {};
class myiclass : public baseclass, public weakly_referencable<myiclass> {};

BOOST_AUTO_TEST_CASE(test_base_class_weak_ptr) {
auto owning_ptr = std::make_unique<myiclass>();

auto get_checker = [] (weak_ptr<baseclass> p) {
return [p = std::move(p)] (bool v) {
BOOST_REQUIRE_EQUAL(bool(p), v);
};
};

auto checker = get_checker(owning_ptr->weak_from_this());
checker(true);
owning_ptr.reset();
checker(false);
}

BOOST_AUTO_TEST_CASE(test_weak_ptr_can_be_moved) {
auto owning_ptr = std::make_unique<myclass>();
weak_ptr<myclass> wp1 = owning_ptr->weak_from_this();
Expand Down