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.

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

Be more correct in our noexcept usage. #14970

Merged
merged 1 commit into from Mar 25, 2023

Conversation

drwells
Copy link
Member

@drwells drwells commented Mar 24, 2023

I noticed this while working on the other one. The default ctor allocates so it's technically not noexcept.

Do we have a policy for this? I don't know if we want to be this pedantic about std::bad_alloc.

Copy link
Member

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

The most important use noexcept is for selecting interfaces the standard library is using and I don't think that this is particularly relevant for Trilinos vectors anyway.

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Out of curiosity, how did you arrive at looking at this? noexcept specifications are both obscure and, arguably, something we haven't cared much about in the past...

Comment on lines +504 to +506
* @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.
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()
{
// 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!

@@ -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

@drwells
Copy link
Member Author

drwells commented Mar 24, 2023

I looked at this because I wanted to see the state of our other vector move constructors after doing #14969. That class also allocates memory by default so its move ctors are not noexcept. IIRC I noticed the issue originally when clang-tidy suggested move ctors always be noexcept.

Why did I do #14969? In a project I have I use my own Scatter class (which just wraps a DoF renumbering and a LA::d::V) to manage two different parallel data partitions (named 'global' and 'overlap'). This leads to code like

{
  Scatter<double> scatter = this->get_scatter(position_dof_handler);
  scatter.global_to_overlap_start(position, 0, overlap_position);
  scatter.global_to_overlap_finish(position, overlap_position);
  this->return_scatter(position_dof_handler, std::move(scatter));
}

I noticed when profiling that this copied instead of moved because we only had one move ctor for LA::d::V (the move ctor, not move assignment operator) so I patched it here and wrote my own workaround there.

@bangerth bangerth merged commit a8c0bd1 into dealii:master Mar 25, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants