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

In step-40, move timer section into the function being timed. #16862

Merged
merged 1 commit into from Apr 7, 2024

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Apr 6, 2024

Each of the other functions declares its timer section itself at the top of the function. Except for output_result(), for which the timer section is inexplicably defined in the run() function. Fix this.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

Yes, this is what I meant in the other comment. Thanks for addressing this. (I should have read all PRs before commenting 😉.)

@kronbichler
Copy link
Member

/jenkins/workspace/dealii-mpi_PR-16862/examples/step-40/step-40.cc:590:26: error: binding reference of type ‘dealii::TimerOutput&’ to ‘const dealii::TimerOutput’ discards qualifiers
  590 |     TimerOutput::Scope t(computing_timer, "output");

@bangerth
Copy link
Member Author

bangerth commented Apr 6, 2024

It wouldn't be the first time I commented on a PR before seeing what else has been submitted that day :-)

I fixed the error (I think) by making output_results() not const. I think that probably also explains the reason why the timer section was outside: We started with a const function and whoever wrote the code thought it was more important to keep the function const than to keep a similar structure between all functions. I think we agree now that that was the wrong decision.

@tamiko tamiko merged commit b2e498c into dealii:master Apr 7, 2024
16 checks passed
@bangerth bangerth deleted the 40-1 branch April 7, 2024 18:28
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

4 participants