Skip to content

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

Merged
krlmlr merged 6 commits intomainfrom
f-1613-pv
Jan 6, 2025
Merged

feat: Use _pv destroy functions to satisfy ASAN checks#1630
krlmlr merged 6 commits intomainfrom
f-1613-pv

Conversation

@krlmlr
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr commented Dec 11, 2024 •
edited
Loading

Part of #1613.

@aviator-app
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
UNPROTECT(9);
}
igraph_vector_int_destroy(&news);
igraph_vector_int_destroy_pv(&news);
Copy link
Copy Markdown
Contributor Author

@krlmlr krlmlr Jan 5, 2025

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
Copy Markdown
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
@krlmlr krlmlr deleted the f-1613-pv branch January 6, 2025 06:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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