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 memory leak of npy_file's move assignment #2719

Closed
wants to merge 1 commit into from

Conversation

qingyunqu
Copy link
Contributor

@qingyunqu qingyunqu commented Aug 10, 2023

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • API of new functions and classes are documented.

Description

Fix memory leak when one npy_file move to another npy_file.

@tdegeus
Copy link
Member

tdegeus commented Nov 9, 2023

Thanks! Sorry for not addressing this sooner. I'm not an expert on this part of the code, but I will try to help. Could you first rebase your PR on the current master in which the CI issues are fixed, such that we can take it from there?

@tdegeus
Copy link
Member

tdegeus commented Nov 9, 2023

Also, could you add a test case with which we could have found this bug?

Copy link
Contributor

github-actions bot commented Jan 9, 2024

This pr is stale because it has been open for 60 days with no activity.
It will be automatically closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 9, 2024
@qingyunqu
Copy link
Contributor Author

Thanks! Sorry for not addressing this sooner. I'm not an expert on this part of the code, but I will try to help. Could you first rebase your PR on the current master in which the CI issues are fixed, such that we can take it from there?

Done!

@qingyunqu
Copy link
Contributor Author

qingyunqu commented Jan 15, 2024

Also, could you add a test case with which we could have found this bug?

This PR fix mem-leak of npy_file's move assignment method. It will leak when:

xt::detail::npy_file a = xt::detail::load_npy_file(fileName);
xt::detail::npy_file b = xt::detail::load_npy_file(fileName);
a = std::move(b); // original memory of a doesn't be freed before move.

@github-actions github-actions bot removed the Stale label Jan 16, 2024
Copy link
Contributor

This pr is stale because it has been open for 60 days with no activity.
It will be automatically closed in 14 days.

@github-actions github-actions bot added the Stale label Mar 17, 2024
@tdegeus
Copy link
Member

tdegeus commented Mar 19, 2024

Also, could you add a test case with which we could have found this bug?

This PR fix mem-leak of npy_file's move assignment method. It will leak when:

xt::detail::npy_file a = xt::detail::load_npy_file(fileName);
xt::detail::npy_file b = xt::detail::load_npy_file(fileName);
a = std::move(b); // original memory of a doesn't be freed before move.

Could you add this to the tests? Thanks!

@tdegeus tdegeus removed the Stale label Mar 19, 2024
Copy link
Contributor

This pr is stale because it has been open for 60 days with no activity.
It will be automatically closed in 14 days.

@github-actions github-actions bot added the Stale label May 19, 2024
Copy link
Contributor

github-actions bot commented Jun 2, 2024

This issue was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this Jun 2, 2024
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

2 participants