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.

Sign up for GitHub

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

Jump to bottom

feat: Use _pv destroy functions to satisfy ASAN checks #1630

Merged
merged 6 commits into from
Jan 6, 2025
Merged

feat: Use _pv destroy functions to satisfy ASAN checks #1630

merged 6 commits into from
Jan 6, 2025

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Dec 11, 2024 •
edited
Loading

Part of #1613.

@aviator-app Aviator app
Copy link
Contributor

aviator-app bot commented Dec 11, 2024 •
edited
Loading

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 11, 2024 •
edited
Loading

ASAN run for the main branch: https://github.com/igraph/rigraph/actions/runs/12274005767/job/34246019696

ASAN run for this branch: https://github.com/igraph/rigraph/actions/runs/12274094491

@krlmlr krlmlr changed the title feat: Use _pv destroy functions to satisfy UBSAN checks feat: Use _pv destroy functions to satisfy ASAN checks Dec 11, 2024
@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 11, 2024

Both checks still fail, it's probably a different IGRAPH_FINALLY() we need to change. The failure is located at line 10243 of the output.

krlmlr
krlmlr commented Jan 5, 2025
View reviewed changes
Copy link
Contributor Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! You converted many more instances than I expected, but that's fine.

src/rinterface_extra.c Outdated
@@ -620,7 +650,7 @@ igraph_error_t R_igraph_attribute_add_vertices(igraph_t *graph, igraph_integer_t
val=VECTOR_ELT(attr, 2);
UNPROTECT(9);
}
igraph_vector_int_destroy(&news);
igraph_vector_int_destroy_pv(&news);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is necessary, the igraph_vector_int_destroy() function is best here. I'll revert that part.

@krlmlr krlmlr marked this pull request as ready for review January 5, 2025 19:32
@krlmlr krlmlr enabled auto-merge (squash) January 5, 2025 19:32
@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 5, 2025

Thanks!

@krlmlr krlmlr disabled auto-merge January 6, 2025 06:33
@krlmlr krlmlr merged commit adbbae0 into main Jan 6, 2025
6 checks passed
@krlmlr krlmlr deleted the f-1613-pv branch January 6, 2025 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@krlmlr @Antonov548