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

[REVIEW] Fix malloc/delete[] mismatch #808

Merged

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Sep 6, 2022

Fix malloc allocations that use delete[] for deallocation, by using std::make_unique<int[]> to manage memory instead.

Fixes #807.

@mhoemmen mhoemmen requested a review from a team as a code owner September 6, 2022 23:32
@github-actions github-actions bot added the cpp label Sep 6, 2022
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 6, 2022
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Looks like a style updated is needed, but this LGTM pending CO.

Thanks for fixing this Mark!

int* in_rows_h = (int*)malloc(params.nnz * sizeof(int));
int* in_cols_h = (int*)malloc(params.nnz * sizeof(int));
int* verify_h = (int*)malloc(params.nnz * sizeof(int));
auto in_rows_h = std::make_unique<int[]>(params.nnz);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just use a std::vector here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjnolet A good point, though I would prefer not to change the test's existing behavior by zero-initializing all the elements of the arrays. (That would also hinder a sanitizer's ability to track use of uninitialized values.) It's up to you, though!

Copy link
Member

Choose a reason for hiding this comment

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

In general I prefer the use of vectors over smart pointers for the host arrays, especially as we are just performing a loop over the array to directly populate it in the next step.

It's not a huge deal in this test, though, as it's pretty trivial.

It's not correct to use delete[] to free a malloc allocation.
This commit changes those allocations to use std::make_unique<int[]>,
so we no longer need manual deallocation.

Fixes rapidsai#807.
@mhoemmen mhoemmen force-pushed the Fix-807-malloc-delete-mismatch branch from 86354f9 to fb4572c Compare September 7, 2022 03:15
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 7, 2022

Thanks @cjnolet ! I just reformatted the modified file. Your formatting script is fantastic, btw! It told me exactly what version of clang-format to use. Different clang-format versions change the formatting a lot, so it's really helpful that the script reports the required version.

@cjnolet
Copy link
Member

cjnolet commented Sep 7, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b5b66a3 into rapidsai:branch-22.10 Sep 7, 2022
@mhoemmen mhoemmen deleted the Fix-807-malloc-delete-mismatch branch September 7, 2022 14:53
@mhoemmen mhoemmen changed the title [WIP] Fix malloc/delete[] mismatch [REVIEW] Fix malloc/delete[] mismatch Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] delete[] being used for malloc'd allocations
2 participants