Dealii: Add move constructors and operators

[tamiko]

The following vector classes currently lack move contructors and move operators

PETScWrappers::MPI::Vector
PETScWrappers::MPI::BlockVector
parallell::distributed::Vector
parallell::distributed::BlockVector

[bangerth]

This is actually a difficult proposition. You need to steal the MPI state from these vectors. How would one implement a move operator for MPI_Comm?

[tamiko]

@bangerth There is always the possibility to just swap underlying data members (without a strict move).

[bangerth]

Good point.

[danshapero]

This feature would be really helpful for me, so I'm going to try and implement it myself.

The move constructor for dealii::Vector just initializes an empty vector and then invokes the swap method on the Vector&& argument passed to the constructor; is there a reason to think that this approach won't work with parallel vectors? The dealii::parallel::distributed::Vector.swap method checks that there are no outstanding requests for updating ghost values first, so maybe checking if we need to call update_ghost_values on the input vector first would be an extra step necessary in the parallel case.

[bangerth]

I see no reason why this shouldn't work, but it may require a bit of trying to figure out how to swap things such as the MPI communicator object.

The extra checks are certainly a good idea.

[bangerth]

This is also part of #1519.

[bangerth]

@danshapero -- looking at a recent patch, another class (in addition to Table) that could use a move constructor is IndexSet. That's one that I'd personally like to be able to return from functions.

[danshapero]

Just from eyeballing it, I think IndexSet could use the default move constructor, but I'll try it today.

I was looking at Table also. If AlignedVector has a move constructor, then the move constructors for Table and then FullMatrix come for free.

[danshapero]

@bangerth -- I looked at this some more and IndexSet cannot use the default operations. I have a working implementation now, but I'm wondering why both of the current constructors set the member largest_range to numbers::invalid_unsigned_int though.

[bangerth]

We use largest_range as an index into the vector of ranges. When the object is default constructed, that vector of ranges is empty, so the index into it needs to be an invalid one (no valid index would make sense as an index into an empty array). Does that make sense?

[danshapero]

Gotcha, I was wondering why it wasn't 0.

[danshapero]

The next few I'd like to tackle are:

• AlignedVector
• TableBase, Table
• internal::FEValues::FiniteElementRelatedData
• FEValuesBase, FEValues

Each one of these is a dependency, either through inheritance or composition, of the next. FullMatrix is also defined in terms of Table, so that could come later.

[bangerth]

That seems like an excellent plan, at the very least the first 2 groups.

As for FEValues: I don't really see a good reason to create and return these objects from a function. We always seem to create them locally in a function and, if necessary, pass them around by reference. So I think there is not that much value.

I _could_ see value in moving triangulations, for example in the functions in namespace GridGenerator. Right now, we always return in some trailing non-const argument. This could be changed if we had move semantics.

[danshapero]

I started writing a move constructor for triangulations, but I'm not quite sure what to do with any attached signals when the triangulation moves. Should the new triangulation retain the signals of the moved one or not? Should a new signal be added that triggers when the triangulation moves?

So far, I can only get it to compile if I do nothing with the Signals attached to the moving object. A signal is non-copyable, but as of boost-1.54 is movable. The default move constructor for Signals is deleted, which is surprising to me as all of its members appear to be movable.

[bangerth]

Conceptually, a signal is used to listen to what happens to _one specific object_. In other words, the signal should not be copied (i.e., the slots should not be attached to the resulting object).

I think it would be reasonable to trigger the clear signal on the source triangulation at the end of the move.

[danshapero]

Thanks @bangerth , that certainly makes things simpler. I guess we know where you stand re: the Ship of Theseus now.

[bangerth]

Ha, interesting conundrum :-) I had never heard of this. My answer is clear indeed -- an object in C++ is characterized primarily through its address in memory, in my view. In other words, you move the _content_, not the _object_.

[bangerth]

It would be great if we could also move FullMatrix objects (e.g., because that's what some functions in FETools return, see #3807). This would require move constructors for Table objects, which should be relatively straightforward to implement.

Any volunteers?

[danshapero]

I implemented move operators for TableBase in #2610; since FullMatrix inherits from Table, the default move operators ought to work fine. In any case, I'm happy to write this.

[bangerth]

That would be great! I don't recall the semantics, but does a class have a move constructor/operator if (i) its bases/members have such operators, (ii) it has other user-declared constructors/operators, (iii) but does not explicitly declare move constructors/operators?

[drwells]

I believe that since the parent class has a move constructor and FullMatrix has no new members that it will automatically be move constructible and move assignable. To check I ran

std::cout << std::boolalpha;
std::cout << std::is_move_assignable<FullMatrix<double>>::value << std::endl;
std::cout << std::is_move_constructible<FullMatrix<double>>::value << std::endl;

and got

true

so it looks like we already have it :)

[drwells]

I don't believe that whether or not the class has other possible constructors changes its move semantics.

[bangerth]

Ah, excellent. Mind making what you have into a testcase that we can add?

There are other cases where adding constructors removes the automatic ones. For example, if you add a constructor that takes at least one argument, then there is no longer a default constructor. But apparently that is not the case for move constructors -- nice!

[danshapero]

@drwells Anything that's copy-constructible is also move-constructible, since copying fulfills all the requirements of moving. Of course that's rarely what you want to happen, so to check whether move semantics are actually used, I've been checking to see if the moved-from object is empty.

[masterleinad]

In fact they are not used:

int main ()
{
deallog.depth_console (0);

std::cout << std::boolalpha;
std::cout << std::is_move_assignable>::value << std::endl;
std::cout << std::is_move_constructible>::value << std::endl;

FullMatrix matrix1 (3,3);
for (unsigned int i=0; i<3; ++i)
for (unsigned int j=0; j<3; ++j)
matrix1(i,j)=i+j;

FullMatrix matrix2 (std::move(matrix1));

matrix2(1,1)=1;

std::cout << &matrix1 << std::endl;
std::cout << &matrix2 << std::endl;

matrix1.print(std::cout);
matrix2.print(std::cout);

return 0;
}

gives

true
true
0x7ffd6d3e2500
0x7ffd6d3e2490
0 1 2
1 2 3
2 3 4
0 1 2
1 1 3
2 3 4

[drwells]

That is good to know. That is a somewhat deceptive type trait.

edit: typo

[bangerth]

It looks to me like we've made good progress on this one, and I don't see any specific omissions from the list of classes where it would make sense to add move constructors. Let's close this as "essentially we did what we wanted to do for this issue". We will certainly add more move constructors over time, but we don't need to keep this issue open for that.