Skip to content

Comments

refactor: Remove -Awarnings from snapshot tests and fix fix_unused_unsafe test#1603

Merged
randomPoison merged 3 commits intomasterfrom
legare/unused-unsafe-test
Feb 23, 2026
Merged

refactor: Remove -Awarnings from snapshot tests and fix fix_unused_unsafe test#1603
randomPoison merged 3 commits intomasterfrom
legare/unused-unsafe-test

Conversation

@randomPoison
Copy link
Contributor

So turns out the reason fix_unused_unsafe wasn't working is because we were suppressing warnings, and the transform relies on the warning analysis to determine what unsafe is unused. I don't really like this, but it doesn't seem like removing -Awarnings breaks anything, and I don't think it's worth building our own unused-unsafe analysis to avoid this.

@kkysen what was the reason for having -Awarnings in there to begin with? Is removing that going to cause any problems I'm not seeing?

@randomPoison randomPoison requested a review from kkysen February 20, 2026 22:16
@randomPoison randomPoison added refactorer This issue relates to the refactoring tool tests Problems with the unit or integration tests labels Feb 20, 2026
@randomPoison randomPoison changed the title Remove -Awarnings from snapshot tests and restore fix_unused_unsafe test refactor: Remove -Awarnings from snapshot tests and restore fix_unused_unsafe test Feb 20, 2026
@kkysen kkysen changed the title refactor: Remove -Awarnings from snapshot tests and restore fix_unused_unsafe test refactor: Remove -Awarnings from snapshot tests and fix fix_unused_unsafe test Feb 20, 2026
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Ahh, that's what was happening. I added -Awarnings because when c2rust-refactor runs rustc, a ton of warnings are printed and these were very noisy (c2rust transpile emits code with a ton of warnings). cargo test shows all of these and doesn't capture the output for some reason like it normally does. However, cargo nextest run does, so it's not that big of an issue if we're using that. So I think removing -Awarnings is okay here.

Could you add to fix_unused_unsafe's docs that it relies on the warnings? That's unexpected, so it's useful to know and call that out.

@randomPoison
Copy link
Contributor Author

Ah yeah, if I run cargo test I see all of the warning output. I was using cargo nextest so I didn't see it. I agree that that's probably fine since we're adopting nextest. I've also added the note to fix_unused_unsafe.

@randomPoison randomPoison requested a review from kkysen February 23, 2026 20:25
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM except for the one minor nit.

Co-authored-by: Khyber Sen <kkysen@gmail.com>
@randomPoison randomPoison merged commit c465678 into master Feb 23, 2026
11 checks passed
@randomPoison randomPoison deleted the legare/unused-unsafe-test branch February 23, 2026 21:21
kkysen added a commit that referenced this pull request Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactorer This issue relates to the refactoring tool tests Problems with the unit or integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants