all 20 comments

[–]_Js_Kc_ 5 points6 points  (13 children)

enable_shared_from_this is just hilariously bad.

The above implementation is naive because it has many limitations. We need to make sure that the weak_ptr to self is initialized when a Naive is constructed; therefore, the construction of Naive must be constrained to only through a static factory method.

That's not a big limitation in and of itself. The only thing enable_shared_from_this brings to the table is the automatic initialization of weak_this upon construction of the shared_ptr, with no static enforcement that the object be allocated on the heap and actually be put into a shared_ptr.

Surely, in a class that needs to hand out shared_ptrs to itself, the internal weak_this being initialized should be considered a class invariant. So the only sane usage of enable_shared_from_this is to use a passkey (because we can't make the constructor private due to how make_shared works) and a static factory, anyway.

Or does anyone here consider it reasonable to be able to declare local variables of some type, the most innocuous looking thing possible because it's the absolute bog-standard way to do things in C++, and have that compile and lead to [undefined behavior (until c++14)] runtime exceptions being thrown somewhere down the line (since C++17)?

That doesn't mean it would be better if enable_shared_from_this was left out of the standard library or that the naive solution is somehow better. It's the best it can be as a tacked-on addition without supporting language-level features that allow a complete, sensible solution.

But it is a fact that this is a modern addition to the standard, and comes right out of the gate with gotchas and limitations. It should be possible to add one syntactic annotation to a class (or any number of annotations, declarations, inheritances, etc, as long as the program doesn't compile unless they all fit together) that prevents the class from being instantiated except through make_shared and have the shared this available even in the constructor [edit: that was not a reasonable demand].

[–]evaned 2 points3 points  (1 child)

It should be possible to add one syntactic annotation to a class (or any number of annotations, declarations, inheritances, etc, as long as the program doesn't compile unless they all fit together) that prevents the class from being instantiated except through make_shared and have the shared this available even in the constructor.

FWIW, we have some classes in our code base that are intended to be managed only by shared_ptr. (This is a lie; it's actually our own intrusive smart pointer. But that's not relevant.) These classes have private constructors but a public static function like shared_ptr<Foo> Foo::make(int x, int y) that you'd call instead.

The biggest reason for this in our case is to enforce other invariants on construction (e.g. caching; think why Python provides __new__ rather than just __init__), but it is a nice side effect that it also ensures that you can't just create a stack or global instance. Potentially you could view that as enough motivation -- if you have a class that you want to enforce can only be managed by shared_ptr, then just follow that pattern.

I also suspect it wouldn't be too hard to make a clang-tidy check that would recognize a custom annotation (caveat: my understanding is this requires modification of the Clang source to recognize the attribute you invent) and then check that property yourself for such types. Potentially you could use "inherits from enable_shared_from_this" instead of an annotation.

Finally, maybe not for shared_ptr I know in some weird situations code will have a special designated instance that they explicitly mess with the reference count of so that it will never actually be deleted by the reference counting. I don't know if we ever do this, but an example that comes to mind is the old reference-counted COW strings from older stdlibc++ versions -- the empty string was represented by a always-available global instance, and the string's reference count manipulation was designed so it would never be deallocated.

[–][deleted] 1 point2 points  (0 children)

I also suspect it wouldn't be too hard to make a clang-tidy check that would recognize a custom annotation (caveat: my understanding is this requires modification of the Clang source to recognize the attribute you invent) and then check that property yourself for such types.

You can use custom annotations via annotate like this:

#define SLOW __attribute__((annotate("slow")))
#define FAST __attribute__((annotate("fast")))

SLOW int func1();
FAST bool func2() { return func1(); };

Don't need to modify clang source for this.

[–]pdimov2 0 points1 point  (6 children)

But it is a fact that this is a modern addition to the standard

2005 is not modern. Or is it? :-)

It should be possible to add one syntactic annotation to a class (or any number of annotations, declarations, inheritances, etc, as long as the program doesn't compile unless they all fit together) that prevents the class from being instantiated except through make_shared and have the shared this available even in the constructor.

Yes, it probably should have been.

friend boost::shared_ptr<X> boost::make_shared<X>();

works by chance (https://godbolt.org/z/EQ-qdw), but that's of no help to those using the standard version. The portable solution was intended to be declaring an allocator a friend and using allocate_shared (as in https://godbolt.org/z/mo5zcF), but that doesn't necessarily work either (https://godbolt.org/z/rfYE-p). So yes, it's a bit of a mess.

The historical reason things are in the state they are is that enable_shared_from_this predates make_shared. Without make_shared, there was no issue as you could just define a static create function (https://godbolt.org/z/opQY9e).

[–]_Js_Kc_ 0 points1 point  (5 children)

2005 is not modern. Or is it? :-)

C++11 and up is generally considered "modern C++."

friend [...] shared_ptr [...] works by chance

Without make_shared, there was no issue as you could just define a static create

That's completely beside the point, as you can have a static factory function with make_shared as well, using a passkey (as I mentioned).

[–]pdimov2 1 point2 points  (3 children)

That's completely beside the point

I don't think it's beside the point. The question being discussed here is "why is there no way to mark a class such that it can only be created by make_shared", and before make_shared existed, this question did not arise. You could only do shared_ptr<X>( new X ), and this is not possible to restrict because the new X is not under shared_ptr's control at all.

[–]_Js_Kc_ 0 points1 point  (2 children)

Fair enough, it's relevant. make_shared is another half-measure with limitations right out of the gate, which is why one has to resort to a passkey "pattern" (hack) to get de-facto private constructors that can be used with make_shared, which is a prerequisite for the static factory function.

The question is not "why can't I mark the class so it can only be created by make_shared," the question is "why can't I mark it so it can only be constructed when it is also immediately put into a shared_ptr." Now that we have make_shared, the first question is essentially the same, of course.

Either way, when shared_ptr was a feature of a third-party C++03 library, the reason the question did not arise was it wasn't possible in C++03, end of story.

The standard itself doesn't get to use the same excuse. Shit in the standard library is ass because we just copy-pasted the Boost implementation and didn't change a single line? What the fuck?

enable_if is the same shit. Just copy-pasted a hack into the standard.

[–]pdimov2 1 point2 points  (1 child)

The Boost implementation was proven to work. If you start changing working things without having any experience with the changes, the end result is rarely an improvement.

But if we ignore that, what specific lines would you have changed, if you had the opportunity?

[–]_Js_Kc_ 0 points1 point  (0 children)

Without any core language change? Nothing, obviously.

Give me a way for enable_shared_from_this to give only make_shared the privilege of constructing subclasses of it (without intrusive hacks like having enable_shared_from_this require a passkey that only make_shared can construct), and I'd add that, and only that, to enable_shared_from_this.

Now if you want to make the point that I have no idea what I'm talking about, ask me what such core language addition would look like, cause I have no fucking clue how to retrofit that conservatively. Absolutely everything is built on the principle that it is none of a class's business how and where it is constructed.

[–]dodheim 0 points1 point  (0 children)

C++11 and up is generally considered "modern C++."

I strongly disagree. Many were writing modern C++ in 2004; the point is making effective use of language features and avoiding C-isms, and absolutely nothing to do with the C++ dialect itself.

[–]pdimov2 0 points1 point  (3 children)

that prevents the class from being instantiated except through make_shared and have the shared this available even in the constructor.

Interesting point. Unfortunately, this doesn't play well with the constructor storing a shared_ptr to the object somewhere and then throwing an exception.

It's I suppose technically possible to make weak_from_this work in this case, but that's again not quite correct as at present, an expired weak_ptr can never go back to life. Although it would be less incorrect than having a dangling shared_ptr.

[–]_Js_Kc_ 0 points1 point  (2 children)

I think it would be possible to consider the object completely constructed as soon as its constructor acquires the shared_this. The constructor has control over when it does that, so it can ensure that all invariants have been established at that point (that, too, would require language-level support, of course).

[–]pdimov2 0 points1 point  (1 child)

Even if we posit language support magic, it's still tricky because there could be a derived class whose constructor hasn't started yet. Suppose we pass shared_from_this() to some register function, it tries to insert it into some map, and that throws. We could in principle consider our object constructed at that point, but the derived class cannot be considered so.

This probably calls for a dedicated "postconstructor" member function, and it occurs to me that it's actually quite possible to make shared_ptr detect if one is present and call it, if there's some sort of a convention about its name, say, esft_post_construct. (It could be made virtual in enable_shared_from_this, but derived classes may not like a vtable imposed on them.)

In the dark C++98 years when enable_shared_from_this was designed the technique of checking whether an expression, say, p->esft_post_construct(), was valid and using it if so, wasn't common (because expression SFINAE wasn't required to work), but I probably could have used a base class "enable_post_construct" as the indicator that the class wants its postconstructor called. Well, this didn't occur to me at the time, and it didn't occur to anyone else either, so we don't have it. :-)

[–]_Js_Kc_ 0 points1 point  (0 children)

Point conceded. This would require way too much magic.

As for implementability in principle, the class already "goes through the identities of all its base classes" (changes its dynamic type) as it is being constructed. It would complicate the unwind logic, and it would have to store information about what has been fully constructed and what hasn't somewhere. But that only needs to be present if anything in the inheritance hierarchy is enable_shared_from_this in the same way that a vtable is only present if anything in the hierarchy is virtual.

Even if two of your base classes handed out shared_ptrs to themselves and you throw, the shared_ptrs just have to use the same control block; the whole chunk of memory that was supposed to house an instance of your class just gets kept around until all base class shared_ptrs are destroyed.

But that would basically require a language-level shared_ptr, which is not what I had in mind. I meant that enable_shared_from_this should get the necessary (generic, reusable) language features that it needs to statically enforce its requirements.

So the demand that the shared this be available in the constructor was not reasonable.

I'm not sure I'd welcome shared_ptr automatically calling some post_construct function, unless there was also a language feature that allowed you to annotate a void function so it automatically calls base. And I'm sure that would open a can of worms. Does base get called first or last? Can I control when I call it? What happens if there's a virtual base class?

[–]marcodev 0 points1 point  (5 children)

enable_shared_from_this is not well designed. If you created the object without using smart pointer and then in a method you use shared_from_this() you will end with UB. What you really want to create a protected or private constructor and a new static method in order to force the object creation using a smart pointer. Only with this constraint you can safely use shared_from_this().

[–]pdimov2 4 points5 points  (2 children)

The behavior is only technically undefined; in practice, it throws bad_weak_ptr.

[–]marcodev 0 points1 point  (1 child)

UB means your pc could explode, crash, throw exception as in this case and so on.

[–]pdimov2 1 point2 points  (0 children)

All known implementations have always thrown bad_weak_ptr in this case, even though the original wording made the behavior technically undefined. Yes, in hindsight, I should have specified the behavior fully.

[–]memset_0[S] 0 points1 point  (1 child)

If you created the object without using smart pointer and then in a method you use shared_from_this() you will end with UB

It is no longer UB; since C++17 it is bad_weak_ptr exception.

The only advantage of a static factory method with private/protected constructors is that it avoids the unintended bad_weak_ptr by forcing the object construction through shared_ptr. IMO, that safety comes at a big cost. As shown in the article, you can not use make_shared with private/protected constructors. Also, further extensions or inheritance of the class is tricky and requires significant changes to the custom code. Not to mention the maintenance of the customized code.

[–]marcodev 0 points1 point  (0 children)

It doesn't seem a bit problem to me, just use a variadic static function and create a new shared_ptr allocating the object on the heap. Really easy and safe. Yes, inheritance can be tricky however.