Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Be more correct in our noexcept usage. #14970

Merged
merged 1 commit into from
Mar 25, 2023
Merged

Be more correct in our noexcept usage. #14970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion include/deal.II/lac/trilinos_vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,12 @@ namespace TrilinosWrappers
/**
* Move constructor. Creates a new vector by stealing the internal data
* of the vector @p v.
*
* @note In order for this constructor to leave the moved-from object in a
* valid state it must allocate memory (in this case, an empty
* Epetra_FEVector) - hence it cannot be marked as noexcept.
Comment on lines +504 to +506
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that Epetra_FEVector objects cannot be moved, only copied? And that that is why we need to allocate memory ourselves? If so, why do we have a move constructor to begin with?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the exact properties of Epetra_FEVectors are but our concern is one level higher - the default constructor for our vector class contains

vector(new Epetra_FEVector(
  Epetra_Map(0, 0, 0, Utilities::Trilinos::comm_self())))

i.e., an invariant maintained by this class is that vector is not nullptr. Hence, even creating an empty object allocates memory.

It's still convenient to have the move ctor since we can swap contents with an allocated (but empty) vector.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see about the invariant.

If the noexcept part were important, you can of course always move the pointer from the rhs object, and then use try{...} catch(...){abort();} to allocate an empty vector for the rhs. But I don't care either way -- we can deal with this whenever someone actually needs these move operators to be noexcept.

*/
Vector(Vector &&v) noexcept;
Vector(Vector &&v); // NOLINT

/**
* Destructor.
Expand Down
4 changes: 3 additions & 1 deletion source/lac/trilinos_vector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,11 @@ namespace TrilinosWrappers



Vector::Vector(Vector &&v) noexcept
Vector::Vector(Vector &&v) // NOLINT
: Vector()
{
// initialize a minimal, valid object and swap
static_cast<Subscriptor &>(*this) = static_cast<Subscriptor &&>(v);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this in essence what you would move clearly express with

Suggested change
static_cast<Subscriptor &>(*this) = static_cast<Subscriptor &&>(v);
static_cast<Subscriptor &>(*this) = std::move(static_cast<Subscriptor &>(v));

or

Suggested change
static_cast<Subscriptor &>(*this) = static_cast<Subscriptor &&>(v);
this->Subscriptor::operator= (std::move(static_cast<Subscriptor &>(v)));

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong feelings either way - I just used the same style I also used in LA::d::V. Mind if I edit the syntax in a followup? std::move() is definitely more readable than the extra &.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of these that can be improved - I'd rather do them all at once in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

swap(v);
}

Expand Down Expand Up @@ -499,6 +500,7 @@ namespace TrilinosWrappers
Vector &
Vector::operator=(Vector &&v) noexcept
{
static_cast<Subscriptor &>(*this) = static_cast<Subscriptor &&>(v);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

swap(v);
return *this;
}
Expand Down