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

MeshBlock Cleanup and Smart Backpointers. Resolves #306. #307

Merged
merged 17 commits into from
Sep 26, 2020

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Sep 18, 2020

PR Summary

As discussed in #306 and #303, smart pointers to meshblocks means we can use them elsewhere in the code, for example, with weak back pointers. This Implements that change.

In addition, I substantially clean up the MeshBlock class. I noticed that all three constructors had the same functionality copy-pasted. I therefore removed all but one of them. I also changed the constructors to a factory method. This was necessary because of a subtlety with smart pointers. You cannot create a weak pointer to an object in its own constructor.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md

@Yurlungur Yurlungur mentioned this pull request Sep 19, 2020
5 tasks
src/mesh/amr_loadbalance.cpp Outdated Show resolved Hide resolved
example/advection/advection_driver.cpp Outdated Show resolved Hide resolved
src/mesh/mesh.hpp Outdated Show resolved Hide resolved
src/mesh/mesh.hpp Outdated Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator Author

@AndrewGaspar I believe all your requested changes have been implemented.

Copy link
Contributor

@AndrewGaspar AndrewGaspar left a comment

Choose a reason for hiding this comment

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

GetBlockPointer is a mouthful, but I'll accept it.

@Yurlungur
Copy link
Collaborator Author

@pgrete @forrestglines can I get one more approval from the Athena team?

@Yurlungur
Copy link
Collaborator Author

GetBlockPointer is a mouthful, but I'll accept it.

Yeah. I know. Block felt too vague, though.

src/interface/container.hpp Outdated Show resolved Hide resolved
src/utils/buffer_utils.hpp Outdated Show resolved Hide resolved
src/utils/buffer_utils.cpp Outdated Show resolved Hide resolved
src/mesh/meshblock.cpp Show resolved Hide resolved
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Looks good and the cleanup of the constructors was well overdue.
Given the extent of the changes I'll trigger the full test now.
Once that passes this can get merged from my point of view.

src/mesh/meshblock.cpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 26, 2020

Codecov Report

Merging #307 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #307      +/-   ##
===========================================
- Coverage    56.71%   56.69%   -0.03%     
===========================================
  Files           99       99              
  Lines        10441    10445       +4     
===========================================
  Hits          5922     5922              
- Misses        4519     4523       +4     
Impacted Files Coverage Δ
example/advection/advection_driver.cpp 100.00% <ø> (ø)
example/advection/advection_package.cpp 98.35% <ø> (ø)
example/calculate_pi/calculate_pi.cpp 100.00% <ø> (ø)
example/kokkos_pi/kokkos_pi.cpp 0.00% <ø> (ø)
src/bvals/boundary_conditions.cpp 29.87% <ø> (ø)
src/bvals/bvals.cpp 74.41% <ø> (+1.24%) ⬆️
src/bvals/bvals.hpp 88.88% <ø> (-11.12%) ⬇️
src/bvals/bvals_interfaces.hpp 90.00% <ø> (-10.00%) ⬇️
src/bvals/bvals_refine.cpp 79.00% <ø> (ø)
src/bvals/bvals_var.cpp 95.23% <ø> (+0.04%) ⬆️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9e1b9d...1431978. Read the comment docs.

@Yurlungur Yurlungur merged commit 0ca951a into develop Sep 26, 2020
@Yurlungur
Copy link
Collaborator Author

Pulling the trigger.

@Yurlungur Yurlungur deleted the jmm/back-pointers branch September 26, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Back pointers to meshblocks should be shared pointers.
4 participants