Use small intrusive pointer in irep

[reuk]

Follows up #685. Rather than using std::shared_ptr which uses more memory than is strictly necessary (an extra pointer to the control block in the shared_ptr itself, plus the control block memory) we use a custom small_shared_ptr which is based on boost's intrusive_ptr.

This approach uses exactly the same amount of memory as was used previously, but separating the resource management into a separate class makes the logic a bit clearer, and greatly simplifies the copy- and move-assignment operators. It also means we don't need to explicitly redeclare custom move/copy constructors/assignment-ops in irep.h.

[tautschnig]

A few questions; I overall like this approach, but what we really need for this to move forward is a solid performance testing set up that has long been talked about.

[tautschnig]

What is the rationale for this change?

[reuk]

See following comment.

[tautschnig]

Same here: keep it simple. We don't need to solve problems we don't have.

[tautschnig]

What's the idea of the separate using directive, instead of just doing std::swap(t_, rhs.t_); ?

[reuk]

In the general case, using std::swap allows you to place a custom swap for your type in a namespace other than std, because manually overloading std::swap isn't allowed. This overload can be found by Koenig lookup, but if not found will fall back to std::swap. In this case, this behaviour isn't strictly required, so I could just use std::swap directly.

[tautschnig]

Keep it simple.

[tautschnig]

What is the use case of this one?

[reuk]

Like std::make_unique and std::make_shared, this has exception-safety benefits.

In the expression my_func(small_shared_ptr<X>(new X), small_shared_ptr<Y>(new Y)), the compiler is allowed to sequence the operations like so:

new X
new Y <-- This may throw and leak previously-allocated X
construct small_shared_ptr<X>
construct small_shared_ptr<Y>
my_func

Whereas my_func(make_small_shared_ptr<X>(), make_small_shared_ptr<Y>()) cannot leak because the calls to make_small_shared_ptr won't be interleaved.

[tautschnig]

Shouldn't that be my_func(make_small_shared_ptr<X, Y>()) ?

[reuk]

No, empty parameter packs are valid. You can see at irep.cpp:30 and irep.h:242 how the function is used in practice.

[tautschnig]

Ok, I think I just misunderstood: your argument was about the order of invoking new vs the construction of small_shared_ptr, and not the order of the small_shared_ptr constructs (which is still unspecified). Fair enough. You may wish to add a comment to the constructor of small_shared_ptr to say that make_small_shared_ptr should be used in case of a new T argument.

What would be a use case where the parameter pack is non-empty? Is this just to support types other than dt that require >= 1 argument in their constructor?

[reuk]

Yes, if you have a type with a constructor like my_type(Foo, Bar, Baz) then you can create a small_shared_ptr to it using make_small_shared_ptr<my_type>(Foo(), Bar(), Baz()) (for example).

This also avoids repetition of the identifier my_type (the alternative would be small_shared_ptr<my_type>(new my_type(Foo(), Bar(), Baz()) which is needlessly repetitive).

[tautschnig]

Ok, thanks!

[tautschnig]

As said before, this looks good to me, and includes quite a bit of long overdue cleanup. Yet I don't dare setting any approval here in absence of a performance test suite.

[tautschnig]

With 22170b7 my looks-good-to-me no longer applies.

[reuk]

Yes, sorry about that - I think the copy-on-write implementation currently in irep is buggy because it allows you to make changes which affect copies in unexpected ways:

irept a;
auto& comments = a.get_comments();
const auto b(a);
comments.<some mutating method>();
// b is changed here!

I'm guessing this is why get_comments and other similar methods are marked DANGEROUS. True copy-on-write would mark the internal structure 'unshareable' when giving out a mutable reference to a data member. Then, the example above becomes:

irept a;
auto& comments = a.get_comments(); // This detaches if necessary, and
                                   // marks the internal data unshareable
const auto b(a); // This is a deep copy because a is unshareable
comments.<some mutating method>(); // Only affects 'a'

I'll close the PR and re-open when I have something that fixes this bug.

[tautschnig]

I don't necessarily think you have to go as far as closing this one. How about making get_comments and get_sub private? There are only a handful of users of those out there.