Overwriting elements doesn't call destructors

[siffiejoe]

Using the assignment operator of nonstd::variant calls constructors, but no destructors.

Example:

#include <cstddef>
#include <iostream>
#include <nonstd/variant.hpp>

namespace {

    struct X {
        X() {
            instances++;
        }
        X(X const&) {
            instances++;
        }
        ~X() {
            instances--;
        }
        static std::ptrdiff_t instances;
    };

    std::ptrdiff_t X::instances = 0;
}

int main() {
    std::cout << X::instances << std::endl;
    {
        nonstd::variant<X> v{};
        std::cout << X::instances << std::endl;
        nonstd::variant<X> w{};
        std::cout << X::instances << std::endl;
        v = w;
        w = v;
        v = w;
        w = v;
        std::cout << X::instances << std::endl;
    }
    std::cout << X::instances << std::endl;
    return 0;
}

Output is:

0
1
2
6
4

I expected it to be:

0
1
2
2
0

[martinmoene]

It conforms to the standard:

19.7.3.3 Variant Assignment p.2.3 on variant& operator=(const variant& rhs) reads:

Let j be rhs.index().

Effects:

• (2.1) If neither *this nor rhs holds a value, there is no effect.
• (2.2) Otherwise, if this holds a value but rhs does not, destroys the value contained in this and sets *this to not hold a value.
• (2.3) Otherwise, if index() == j, assigns the value contained in rhs to the value contained in *this.
• (2.4) Otherwise, if either is­nothrow­copy­constructible­v is true or is­nothrow­move­constructible­v is false, equivalent to emplace(get(rhs)).
• (2.5) Otherwise, equivalent to operator=(variant(rhs)).

[siffiejoe]

I think assignment would be acceptable, so will destruction followed by copy construction. But copy construction alone -- like it is now -- leaks resources.

[martinmoene]

You're right, it's wrong.

Fixes to follow...

[martinmoene]

Todo:

• [x] Change tests tagged .issue-31 into normal test cases

[siffiejoe]

There is another issue, and it's related to the comment I gave in one of the commits. Whenever you write something like

            type_index = helper_type::move( other.type_index, tmp.ptr(), ptr() );
            tmp.type_index = variant_npos_internal();

the code is broken because tmp still contains an object that must be destructed. This is especially true if the contained type doesn't have a move constructor in which case the copy constructor is used instead ...

[martinmoene]

There is another issue, ... the code is broken because tmp still contains an object that must be destructed. This is especially true if the contained type doesn't have a move constructor in which case the copy constructor is used instead ...

Thanks, had noticed that too.

[siffiejoe]

Looks better now, and all my tests are green. I do have some other minor points, though:

• a throwing helper_type::copy_assign (or move_assign) should not reset the type index to npos
• on the other hand, a throwing copy_construct or move_construct should do exactly that
• I'm not sure where the definition of std::exception should come from when variant_CONFIG_NO_EXCEPTIONS is 1, but it seems to work in practice.
• I'm not convinced the temporary copies tmp in variant::copy_assign and variant::move_assign have any effect on exception safety ...

[martinmoene]

• a throwing helper_type::copy_assign (or move_assign) should not reset the type index to npos
• on the other hand, a throwing copy_construct or move_construct should do exactly that

Agreed, C++ working draft, cppreference

[martinmoene]

• I'm not sure where the definition of std::exception should come from when variant_CONFIG_NO_EXCEPTIONS is 1, but it seems to work in practice.

Indeed bad_variant_access must be guarded by variant_CONFIG_NO_EXCEPTIONS.

[martinmoene]

I'm not convinced the temporary copies tmp in variant::copy_assign and variant::move_assign have any effect on exception safety ...

It's perhaps not well formulated. The idea is to prevent the variant to become valueless-by-exception on an exception due to copy/move construction.

For copy construction one copy must be made anyway. In C++11 this can be done before destruction of the assignee, followed by move-construction. Pre-C++11 requires another copy-construction in this scenario. This pessimization for Pre-C++11 has been removed.

For move-construction the double-move pessimization has been removed.

[martinmoene]

@siffiejoe , many thanks for your contributions!

[siffiejoe]

For copy construction one copy must be made anyway. In C++11 this can be done before destruction of the assignee, followed by move-construction. Pre-C++11 requires another copy-construction in this scenario.

This only works if move-construction is actually available and non-throwing. But exception safety seems to be tough for variants. E.g. the current swap method is definitely not noexcept (there's one copy construction and two copy assignments involved).

I think I found another glitch: Shouldn't the copy-assign-value operators be available for C++11 as well?

[siffiejoe]

I also think that helper::move_construct should use std::move instead of std::forward because the result of operator* always is an lvalue, to the copy constructor would be called instead of the move constructor.

[martinmoene]

I think I found another glitch: Shouldn't the copy-assign-value operators be available for C++11 as well?

Argh! Thanks!

Somehow managed to miss that variant's specification has a forwarding reference, not an rvalue reference (on cppreference):

template<typename T> variant& operator=(T&&) versus variant& operator=(T&&)

[martinmoene]

For copy construction one copy must be made anyway. In C++11 this can be done before destruction of the assignee, followed by move-construction. Pre-C++11 requires another copy-construction in this scenario.

This only works if move-construction is actually available and non-throwing. But exception safety seems to be tough for variants. E.g. the current swap method is definitely not noexcept (there's one copy construction and two copy assignments involved).

Note: There's a related issue for nonstd::optional (code).

Moved to issue #34.

[martinmoene]

I also think that helper::move_construct should use std::move instead of std::forward because the result of operator* always is an lvalue, to the copy constructor would be called instead of the move constructor.

Looks like there are several more:

• variant & operator=( variant && other )
• variant & operator=( Tx && t )
• variant & move_assign_value( T && value )

[martinmoene]

@siffiejoe Re: copy-construct + move in copy_assign(): a failing move after a succeeding copy looks less likely to me. Is that a valid thought?

[siffiejoe]

Regarding std::forward and the two move-assign operators: std::move is certainly better, but std::forward wasn't wrong because it was applied to rvalue references. Regarding copy-construct+move: move has to purposes, to be more efficient than copy and to be noexcept. So if the move constructor is actually available, a failing move is less likely, otherwise it's the same as another copy.

[martinmoene]

Regarding std::forward and the two move-assign operators: std::move is certainly better, but std::forward wasn't wrong because it was applied to rvalue references.

In my experience, using std::forward with an rvalue reference instead of using std::move is regarded 'stylistically wrong'.

[siffiejoe]

Since the original issue has been resolved, I think this can be closed. Thanks for all the other improvements!

[martinmoene]

@siffiejoe Thanks for the significant contributions you made!