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

syntax: add debug_assert! that is only active with --cfg debug. #7869

Closed
wants to merge 1 commit into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Jul 18, 2013

No description provided.

@huonw
Copy link
Member Author

huonw commented Jul 18, 2013

@thestinger has pointed out that this should probably abort not fail!, because otherwise the behaviour of the code changes between debug and release configurations. (E.g. someone could accidentally rely on/use debug_assert!'s behaviour. E.g. checking that an SQL statement only has SELECT; bam, attack vector when the assertion disappears for a release build.)

Unless someone has better argument for not changing to abort, I'll do that.

@thestinger
Copy link
Contributor

@huonw: your example would still be relevant if it aborted, you could add a check you depend on and it would be still be gone with optimizations

You can catch a failure at a task boundary though, so it means you could accidentally be silencing your debugging asserts, and it doesn't ever make sense to do that.

@huonw
Copy link
Member Author

huonw commented Jul 18, 2013

@thestinger debug_assert failing would still pass a #[should_fail] test, but an abort wouldn't.

@thestinger
Copy link
Contributor

@huonw: I'll r+ if you switch it to printing out an error and then calling abort()

@graydon
Copy link
Contributor

graydon commented Jul 23, 2013

I don't understand how fail vs. abort figures into it. Doesn't it get compiled-out in all cases when you don't pass --cfg debug?

@bblum
Copy link
Contributor

bblum commented Jul 30, 2013

Can we get a test_assert! counterpart to this that is only executes the contained code when cfg(test)?

@emberian emberian mentioned this pull request Jul 31, 2013
@thestinger
Copy link
Contributor

@graydon: the issue is that it should be clearly for debugging, and if it fails you can accidentally catch the failure at a task boundary and miss it

@emberian
Copy link
Member

emberian commented Aug 7, 2013

@huonw mind updating this?

@huonw
Copy link
Member Author

huonw commented Aug 8, 2013

Closing this for now to remove from bors' queue; I'll try to get back to it soon (maybe, hopefully).

@huonw huonw closed this Aug 8, 2013
@emberian emberian mentioned this pull request Aug 11, 2013
@brson
Copy link
Contributor

brson commented Sep 3, 2013

I currently disagree that this should abort.
I can sort of understand the thinking here that the semantics change a little when not using --cfg debug, but task boundaries are specifically for recovering from unexpected failures.

@huonw huonw deleted the db-assert branch December 4, 2014 02:02
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 4, 2021
…shearth

Update `str` utils to prevent ICEs and FNs

This PR reworks some string handling for lints regarding enum naming. I hope the refactoring will prevent future ICEs and help with new bug free implementations.

It might be better to review this PR by going through the commits, as `clippy_utils::camel_case` was renamed to `clippy_utils::str_utils` and then changed further. GH sadly doesn't really make the changes that obvious 🙃

Not too much more to say. Have a nice day 🌞

---

Fixes: rust-lang/rust-clippy#7869

changelog: ICE Fix: [`enum_variant_names`] rust-lang#7869
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.

6 participants