enable_shared_from_this is a timebomb

[isk...@googlemail.com]

{ Please limit your text to fit within 80 columns, preferably around 70,
so that readers don't have to scroll horizontally to read each line.
This article has been reformatted manually by the moderator. -mod }

I found that enable_shared_from_this is not only restricting way of
creating instance of a class but also leads to obscure design. Usual
way of using it is when class has some active object and passes self
reference to it to listen notification from this object. First of all
this can be always refactored using pimpl idiom (create shared_ptr
<ListenerImpl>(new ListenerImpl(this)) and pass pimpl reference to
active object. It makes intention clear. One more benefit is that this
approach doesn't restrict possible ways of creation.

The problem with enable_share_from_this is that designer of a class
should provide factory method which returns shared_ptr and this should
be the only way to create instance of the class. But this never happens
- nobody does it! It's very easy to overlook enabled_shared_from_this
in inheritance list or just forget about its restriction and create
instance on stack. This even can work for sometime - it's like giving
a gun to baby and watch what will happen.

So why should somebody use enable_shared_from_this?


--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

[goran...@googlemail.com]

On Thursday, October 18, 2012 12:30:02 AM UTC+2, (unknown) wrote:
> { Please limit your text to fit within 80 columns, preferably around 70,
> so that readers don't have to scroll horizontally to read each line.
> This article has been reformatted manually by the moderator. -mod }

{ The same applies to this post. -mod }

>
> I found that enable_shared_from_this is not only restricting way of
> creating instance of a class but also leads to obscure design. Usual
> way of using it is when class has some active object and passes self
> reference to it to listen notification from this object. First of all
> this can be always refactored using pimpl idiom (create shared_ptr
> <ListenerImpl>(new ListenerImpl(this)) and pass pimpl reference to
> active object. It makes intention clear. One more benefit is that this
> approach doesn't restrict possible ways of creation.
>
> The problem with enable_share_from_this is that designer of a class
> should provide factory method which returns shared_ptr and this should
> be the only way to create instance of the class. But this never happens
> - nobody does it! It's very easy to overlook enabled_shared_from_this
> in inheritance list or just forget about its restriction and create
> instance on stack. This even can work for sometime - it's like giving
> a gun to baby and watch what will happen.
>
> So why should somebody use enable_shared_from_this?

By your logic, one should dismiss shared_ptr altogether, because,
as you say, one can create shared_ptr from an address of an automatic
(stack) object, and shoot oneself in a foot.

The explanation over here:
http://en.cppreference.com/w/cpp/memory/enable_shared_from_this is
pretty clear, and it notes one possible wrong use.

Your pimpl idea does not work in a general case. Your ListenerImpl
presumes taking ownership of "this", which is something that needs to
enforced through code (what if "this" is already inside a shared_ptr,
unique_ptr, if it is a class member or an automatic/static object?).

I think you just have been burned by a wrong use and want to axe it
altogether out of excitement ;-).

I agree about adding a factory function (shared_ptr<TYPE>
CreateTYPE(...)) and hiding the constructor(s) and "operator new" for
types that are supposed to exclusively be used with shared_ptr.


Goran.

[Martin Bonner]

{ Reformatted; please, pretty please with sugar on top, limit your
lines to 70 characters - mod }

On Wednesday, October 17, 2012 11:30:02 PM UTC+1,

isk...@googlemail.com wrote:

> I found that enable_shared_from_this is not only restricting way of
> creating instance of a class but also leads to obscure design. Usual
> way of using it is when class has some active object and passes self
> reference to it to listen notification from this object. First of
> all this can be always refactored using pimpl idiom (create
> shared_ptr <ListenerImpl>(new ListenerImpl(this)) and pass pimpl
> reference to active object. It makes intention clear. One more
> benefit is that this approach doesn't restrict possible ways of
> creation.

I don't see how this works. Say I have a class Foo. Objects of this
class are owned by shared_ptrs. If I want to pass a shared ptr to
some part of Foo into a listener (and I want the listener to keep my
Foo alive), how would constructing a separate shared_ptr help? I want
a single reference count, and only when this reaches zero, do I want
the object to disapper.

> The problem with enable_share_from_this is that designer of a class
> should provide factory method which returns shared_ptr and this
> should be the only way to create instance of the class. But this

> never happens - nobody does it! I have an application where the
> factory function needs to be a member of another class. I could
> make the creating class a friend of the created, or I could create
> another factory function, but it really doesn't seem worth it. I'm
> just not that worried about people creating objects of my class on
> the stack.

> It's very easy to overlook enabled_shared_from_this in inheritance
> list or just forget about its restriction and create instance on
> stack.

That hasn't been my experience. Wherever I have used shared_ptr it
has been the case that *every* instance of the class is controlled by
a shared_ptr. It just wouldn't make sense to create one on the stack.

> So why should somebody use enable_shared_from_this?

Err. Whenever the class is expected to be owned by a set of
shared_ptrs, and the class wants to provide a shared_ptr to itself,
either as an argument to a function, or as a return value.

[abr...@googlemail.com]

> The problem with enable_share_from_this is that designer of a class
> should provide factory method which returns shared_ptr and this should
> be the only way to create instance of the class. But this never happens
> - nobody does it! It's very easy to overlook enabled_shared_from_this
> in inheritance list or just forget about its restriction and create
> instance on stack. This even can work for sometime - it's like giving
> a gun to baby and watch what will happen.

I use it (http://sourceforge.net/projects/asio-samples/) in this manner:

Declaration (*.hpp):

class session
: private boost::noncopyable
, public boost::enable_shared_from_this<session>
{
private:
typedef session this_type;
...
public:
static session_ptr create(boost::asio::io_service& io_service,
const session_config& config);
...
protected:
session(boost::asio::io_service&, const session_config&);
~session();
...
}; // class session

Definition (*.cpp):

...
session_ptr session::create(boost::asio::io_service& io_service,
const session_config& config)
{
typedef shared_ptr_factory_helper<this_type> helper;
return boost::make_shared<helper>(boost::ref(io_service), config);
}
...

Template class shared_ptr_factory_helper is used to expose protected
ctor/dtor of session class (I can't use variadic templates because of
MSVC 10.0):

template <typename T>
struct shared_ptr_factory_helper : T
{
shared_ptr_factory_helper()
: T()
{
}

template <typename Arg1>
explicit shared_ptr_factory_helper(Arg1&& arg1)
: T(std::forward<Arg1>(arg1))
{
}

template <typename Arg1, typename Arg2>
shared_ptr_factory_helper(Arg1&& arg1, Arg2&& arg2)
: T(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2))
{
}

template <typename Arg1, typename Arg2, typename Arg3>
shared_ptr_factory_helper(Arg1&& arg1, Arg2&& arg2, Arg3&& arg3)
: T(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2),
std::forward<Arg3>(arg3))
...
}; // struct shared_ptr_factory_helper

[Martin Bonner]

On Friday, October 19, 2012 8:50:02 PM UTC+1, Martin Bonner wrote:
> { Reformatted; please, pretty please with sugar on top, limit your
> lines to 70 characters - mod }

Personally I think our esteemed moderator has better things to do
with his time than reformat the posts of lazy idiots who can't be
bothered to get a simple thing like this right.

I would certainly support simply rejecting such posts.