weak_ptr: Make it possible to produce constant pointers #2226
Conversation
|
The ultimate intent is to apply --- a/cql3/statements/prepared_statement.hh
+++ b/cql3/statements/prepared_statement.hh
@@ -33,7 +33,7 @@ struct invalidated_prepared_usage_attempt {
class prepared_statement : public seastar::weakly_referencable<prepared_statement> {
public:
- typedef seastar::checked_ptr<seastar::weak_ptr<prepared_statement>> checked_weak_ptr;
+ typedef seastar::checked_ptr<seastar::weak_ptr<const prepared_statement>> checked_weak_ptr;
public:
const seastar::shared_ptr<cql_statement> statement;
@@ -51,7 +51,7 @@ class prepared_statement : public seastar::weakly_referencable<prepared_statemen
prepared_statement(seastar::shared_ptr<cql_statement>&& statement_);
- checked_weak_ptr checked_weak_from_this() {
+ checked_weak_ptr checked_weak_from_this() const {
return checked_weak_ptr(this->weak_from_this());
}
};to Scylla, since prepare_statements are in fact immutable throughout their lifetime |
|
:( |
include/seastar/core/weak_ptr.hh
Outdated
|
|
||
| weak_ptr<const T> weak_from_this() const noexcept { | ||
| weak_ptr<const T> ptr(static_cast<const T*>(this)); | ||
| _ptr_list.push_back(reinterpret_cast<weak_ptr<T>&>(ptr)); |
There was a problem hiding this comment.
Instead of this problematic construct (as you saw, the compiler didn't like it either), I wonder if you can make the _ptr_list definition use something like const T or std::remove_const<T> instead of T, which will maybe allow you to push the weak_ptr onto _ptr_list.
(I haven't thought this through so maybe you can come up with a better solution)
There was a problem hiding this comment.
I thought about it too, but couldn't break the boost intrusive list requirement to explicitly specify the type of objects it keeps :( I pushed an updated version that made the compiler happy
xemul
changed the title
xemul
changed the title
the workflow is configured to be triggered with |
| weak_ptr<const T> ret(std::exchange(tmp._ptr, nullptr)); | ||
| tmp._hook.swap_nodes(ret._hook); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
@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:
- more standard compliant. so easier to learn from a regular C++ programmer's perspective
- we get a better
weak_ptr, as casting to a compatible pointer type should be possible, and this behavior is more intuitive. - 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
upd:
|
|
@scylladb/seastar-maint hello maintainers, could you help merge this change? |
|
upd:
|
The patch introduces weak_ptr<T>(weak_ptr<Y>&&) constructor that creates weak_ptr<T> out of convertible to T* Y's. This has two implications. First, the main one, is that it's now possible to obtain a constant weak pointer on an object. Another, a nice side effect, is: given a base-class we can now create weak_ptr<base> out of weak_ptr<inherited>. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
environment issue? (@tchaikov ) |
Will take a look. |
| public: | ||
| template <typename U> | ||
| requires std::convertible_to<U*, T*> | ||
| weak_ptr(weak_ptr<U>&& o) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I thought it would know to convert references. But it's just cosmetic anyway.
There was a problem hiding this comment.
OK. Is there anything else that prevents this patch from being merged? :)
|
Looks good. |
|
@scylladb/seastar-maint hello maintainers, could you help merge this change? |
The patch introduces weak_ptr(weak_ptr&&) constructor that creates
weak_ptr out of convertible to T* Y's. This has two implications.
First, the main one, is that it's now possible to obtain a constant weak
pointer on an object.
Another, a nice side effect, is: given a base-class we can now create
weak_ptr out of weak_ptr.