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

[c++] Fix memory-management issues in new domainish helpers #3030

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Sep 22, 2024

Issue and/or context: Found while debugging memory-management issues on #3027 for #2407 / [sc-51048].

Changes:

The new libtiledbsoma helpers used by #3027 were incorrectly pairing new and free, as immediately revealed by my running valgrind build/libtiledbsoma/test/unit_soma while debugging some issues on #3027. There are code comments saying to use new to match the releaser callbacks, but these are incorrect -- the releaser callbacks are using free.

I also took the opportunity to check the ArrowArray and ArrowSchema structs and ensure I was setting all missing attributes to 0 or nullptr, respectively.

Notes for Reviewer:

It might be simpler to make use of memset(arrow_array, 0, sizeof(ArrowArray) and memset(arrow_schema, 0, sizeof(ArrowSchema) right after the allocs, to avoid all the onesie-twosie setting of non-relevant attributes to 0 or nullptr, respectively.

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.79%. Comparing base (11d5396) to head (937ee36).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3030      +/-   ##
==========================================
+ Coverage   89.64%   89.79%   +0.14%     
==========================================
  Files          40       40              
  Lines        4096     4096              
==========================================
+ Hits         3672     3678       +6     
+ Misses        424      418       -6     
Flag Coverage Δ
python 89.79% <ø> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 89.79% <ø> (+0.14%) ⬆️
libtiledbsoma ∅ <ø> (∅)

@johnkerl johnkerl force-pushed the kerl/table-utils-memory branch 5 times, most recently from ccef432 to 25b2a47 Compare September 22, 2024 02:17
Copy link
Member

@nguyenv nguyenv 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 for make all these consistently malloc/free.

@johnkerl johnkerl merged commit 415f1a2 into main Sep 23, 2024
15 checks passed
@johnkerl johnkerl deleted the kerl/table-utils-memory branch September 23, 2024 16:59
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.

2 participants