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

Fix warnings using clang 13.0 #12299

Merged
merged 1 commit into from
May 27, 2021
Merged

Fix warnings using clang 13.0 #12299

merged 1 commit into from
May 27, 2021

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented May 25, 2021

#12294 does not fix all the warnings from clang 13.0. This fixes the others.

masterleinad
masterleinad reviewed May 25, 2021
View reviewed changes
include/deal.II/dofs/dof_accessor.h Outdated
Comment on lines 286 to 290
/**
* Copy constructor.
*/
DoFAccessor(const DoFAccessor<structdim, dim, spacedim, level_dof_access> &) =
default;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to implicitly delete the move constructor? I would prefer to just go with the rule of 5.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also be in favor to adhere to the rule of five, and additionally specify move constructors and copy and move assignment operators like in #12294.

masterleinad
masterleinad reviewed May 25, 2021
View reviewed changes
include/deal.II/grid/tria_accessor.h Outdated
/**
* Move assignment operator.
*/
void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void
CellAccessor<dim, spacedim> &

include/deal.II/grid/tria_accessor.h Outdated
@@ -2868,6 +2883,12 @@ class CellAccessor : public TriaAccessor<dim, dim, spacedim>
void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void
CellAccessor<dim, spacedim> &

@Rombur Rombur changed the title Fix warnings using clang 13.0 [WIP] Fix warnings using clang 13.0 May 25, 2021
@Rombur Rombur force-pushed the clang_13_warnings branch 4 times, most recently from fa4353b to 7409336 Compare May 25, 2021 22:10
@Rombur Rombur changed the title [WIP] Fix warnings using clang 13.0 Fix warnings using clang 13.0 May 25, 2021
@Rombur Rombur removed the WIP ⚠️ label May 25, 2021
@Rombur
Copy link
Member Author

Rombur commented May 25, 2021

This is ready for review. They were a few classes that triggered the warning that I missed the first time.

@Rombur
Copy link
Member Author

Rombur commented May 26, 2021

/rebuild

@Rombur
Copy link
Member Author

Rombur commented May 26, 2021

hmmm looks like we can't get osx and clang-tidy to be happy at the same time. clang-tidy complains that the move constructors are not marked noexcept but I had to remove noexcept for the OSX build to pass. I don't need to add the move constructors to fix the warnings from clang 13.0, so I think the best would be to remove them. Is that OK with you @masterleinad @marcfehling ?

@masterleinad
Copy link
Member

What are the error messages with OSX? I would rather use // NOLINT to suppress clang-tidy complaints (or use the rule of zero if possible).

@Rombur
Copy link
Member Author

Rombur commented May 26, 2021

The code does not compile because when the object is moved into a std::vector it expects a move constructor or a move assignment without noexcept. The compiler refuses to use the functions with noexcept.

masterleinad
masterleinad approved these changes May 26, 2021
View reviewed changes
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.

We could probably apply the rule of zero in step-47 but I don't feel strongly about it.

examples/step-47/step-47.cc
Comment on lines 316 to +330
CopyData(const CopyData &) = default;


CopyData(CopyData &&) = default;


~CopyData() = default;


CopyData &operator=(const CopyData &) = default;


CopyData &operator=(CopyData &&) = default;


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CopyData(const CopyData &) = default;
CopyData(CopyData &&) = default;
~CopyData() = default;
CopyData &operator=(const CopyData &) = default;
CopyData &operator=(CopyData &&) = default;

@Rombur Rombur force-pushed the clang_13_warnings branch 3 times, most recently from 107c1ef to 750cf31 Compare May 26, 2021 17:48
@tamiko
Copy link
Member

tamiko commented May 26, 2021

Let's get this to the release branch as well.

drwells
drwells reviewed May 26, 2021
View reviewed changes
include/deal.II/dofs/dof_accessor.h
* Move assignment operator.
*/
DoFAccessor<structdim, dim, spacedim, level_dof_access> &
operator=(DoFAccessor<structdim, dim, spacedim, level_dof_access>
Copy link
Member

Choose a reason for hiding this comment

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

Does this work with older versions of clang? I expect that we would need a NOLINT here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently have a bunch of places in the library with noexcept for move constructor/assignment operator. I suspect the noexcept problem triggers when a std container calls move but it's just a guess. Not sure if it matters if we are using libc++ or libstc++. The CI OSX did trigger the bug in my PR, so I was hoping that it would catch it again.

@masterleinad masterleinad merged commit 2ee2ec4 into dealii:master May 27, 2021
peterrum added a commit that referenced this pull request May 28, 2021
@peterrum
Merge pull request #12318 from Rombur/clang_13_warnings_93
712d73f 
[9.3] Take over #12299
@Rombur Rombur deleted the clang_13_warnings branch May 25, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drwells drwells drwells left review comments

@marcfehling marcfehling marcfehling left review comments

@masterleinad masterleinad masterleinad approved these changes

@tamiko tamiko tamiko approved these changes

@bangerth bangerth bangerth approved these changes

Assignees
No one assigned
Labels
ready to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Rombur @masterleinad @tamiko @drwells @bangerth @marcfehling