-
Notifications
You must be signed in to change notification settings - Fork 192
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
Rewrite BoundaryHistory to support substeps #5461
Conversation
4cd89a7
to
595f2b3
Compare
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 like the rewrite and I have no requested changes.
- clang-tidy has flagged some moves of trivially-copyable type
- gcc11 is not compiling the print function, maybe need a
using ::operator<<;
or an include in the test?
595f2b3
to
6d72404
Compare
I can reproduce the gcc 11 problem locally, but I'm not having any luck fixing it. Adding |
6d72404
to
45065a6
Compare
gcc 11 fixed. Search for |
Hi @wthrowe some of the clang-tidy errors are ones we have fixed in the past
|
45065a6
to
3b6dcd4
Compare
Disabled some checks in a new commit, fixed the rest. Also deleted an internal function that was never used. |
Leaving out the move constructor and assignment operator is not the same as either explicitly defaulting or deleting them, so this error is difficult to act on.
This does not include support for substeps. That will be added later.
3b6dcd4
to
3a1f668
Compare
3a1f668
to
5832b98
Compare
I confirmed with @kidder that this can get merged |
If reviewers prefer, the second and third commits could be moved to a separate PR.
Proposed changes
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments