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

Fix a memory leak in axom::Array copy constructor #1482

Merged
merged 5 commits into from
Dec 31, 2024

Conversation

publixsubfan
Copy link
Contributor

Summary

  • Fixes leak in axom::Array copy constructor, along the lines of the fix in commit ae8e8c7
  • Add unit test for leaky copy/move constructors and assignment operators

@publixsubfan publixsubfan added bug Something isn't working Core Issues related to Axom's 'core' component labels Dec 26, 2024

TEST(core_array, regression_array_move)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennyweiss -- I replaced the regression test you added in #1286 with a unit test that I think should capture all the leaky cases you were seeing. Let me know if you'd rather keep the regression test as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @publixsubfan -- it looks like it covers similar/more ground, so I'm ok w/ removing the old regression case.

Did you check the bugfix against lsan and/or valgrind or a similar tool?

@publixsubfan publixsubfan force-pushed the bugfix/yang39/mem-leak branch from 34fbe64 to a6f5335 Compare December 27, 2024 00:28
Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

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

Thanks @publixsubfan

Copy link
Contributor

@gunney1 gunney1 left a comment

Choose a reason for hiding this comment

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

Thank you @publixsubfan

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @publixsubfan -- please update the RELEASE-NOTES to reflect the bugfix


TEST(core_array, regression_array_move)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @publixsubfan -- it looks like it covers similar/more ground, so I'm ok w/ removing the old regression case.

Did you check the bugfix against lsan and/or valgrind or a similar tool?

@publixsubfan
Copy link
Contributor Author

@kennyweiss - yep, I tested with an lsan build with and without the bugfix commit.

@publixsubfan publixsubfan force-pushed the bugfix/yang39/mem-leak branch from a5c9634 to c9101fe Compare December 30, 2024 20:40
@publixsubfan publixsubfan merged commit 379e875 into develop Dec 31, 2024
13 checks passed
@publixsubfan publixsubfan deleted the bugfix/yang39/mem-leak branch December 31, 2024 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Core Issues related to Axom's 'core' component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants