-
Notifications
You must be signed in to change notification settings - Fork 707
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
or
Suggested change
? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your call. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! |
||||||||||
swap(v); | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -499,6 +500,7 @@ namespace TrilinosWrappers | |||||||||
Vector & | ||||||||||
Vector::operator=(Vector &&v) noexcept | ||||||||||
{ | ||||||||||
static_cast<Subscriptor &>(*this) = static_cast<Subscriptor &&>(v); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||||||||||
swap(v); | ||||||||||
return *this; | ||||||||||
} | ||||||||||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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_FEVector
s are but our concern is one level higher - the default constructor for our vector class containsi.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.
There was a problem hiding this comment.
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 usetry{...} 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 benoexcept
.