search

scipopt / russcip

Rust interface for SCIP
https://crates.io/crates/russcip
Apache License 2.0
37 stars 11 forks source link

Reference counted ScipPtr #139

Closed mmghannam closed 1 week ago

mmghannam commented 6 months ago

Fixes #138

mmghannam commented 6 months ago

@Nudelmeister turns out it was easy to do the change 😄 I would really appreciate a review here if you have the time :)

Update: there's a memory leak not sure why yet.

2nd Update: Fixed, was most probably because of thread unsafe counting of scip ptr references.

3rd Update: Was a false positive apparently :/

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 76.13%. Comparing base (b44bc33) to head (58fc879).

:exclamation: Current head 58fc879 differs from pull request most recent head 3d5df58. Consider uploading reports for the commit 3d5df58 to get more accurate results

Files Patch % Lines
src/solution.rs 90.91% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #139 +/- ## ========================================== + Coverage 76.02% 76.13% +0.10% ========================================== Files 13 13 Lines 1785 1797 +12 ========================================== + Hits 1357 1368 +11 - Misses 428 429 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Nudelmeister commented 6 months ago

This seems fine, though I thought of wrapping the whole ScipPtr struct in an Arc. I can take a closer look tomorrow.

Nudelmeister commented 5 months ago

You could use a AtomicUsize instead of RwLock<usize>, but looking at how Arc is implemented it's more complicated than I thought. Other than the general uneasiness of the large amounts of unsafe (I guess you can't really do anything about that), I'm happy with this fix.

Thanks for the quick response.

mmghannam commented 5 months ago

You could use a AtomicUsize instead of RwLock<usize>, but looking at how Arc is implemented it's more complicated than I thought. Other than the general uneasiness of the large amounts of unsafe (I guess you can't really do anything about that), I'm happy with this fix.

Thanks for the suggestion, I changed it :) There's still a memory leak, I will merge this once I figure out why and fix it.

Thanks for the quick response.

Thank you too for the quick review :)