-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Let qualify_min_const_fn
deal with drop terminators
#12681
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
Conversation
I do wonder what rust-clippy/clippy_lints/src/missing_const_for_fn.rs Lines 154 to 158 in f5e2501
already_const before this, so I don't see how is_const_fn_raw() could be true.
|
☔ The latest upstream changes (presumably #12713) made this pull request unmergeable. Please resolve the merge conflicts. |
The whole error returning part of |
I assume you mean specifically |
Replacing the loops with |
I was trying to clean some of the error stuff up but found that the error reason strings seemed useful (even just for documentation purposes, maybe they should be comments), so it felt a bit bad to remove that. So I just left it for now and just removed the unused error handling in
Also I must be imagining this differently because it doesn't seem as simple, or at least not as nice as simply changing the error type in McfResult to some ZST struct (which would allow us to just continue using |
The lack of It will be less work to just use a unit error type, but that's only because it's already written. Again, I don't really care which way you go here. It's unrelated to the bug being fixed. |
@bors r+ I'm going ahead and merging this since it's fine as is and fixes a bug. Any extra cleanup can be done with another PR. |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #12677
The
method_accepts_droppable
check that was there seemed overly conservative.Accepting parameters that implement
Drop
should still be fine as long as the parameter isn't actually dropped, as is the case in the linked issue where the droppable is moved into the return place. This more accurate analysis ("is there adrop
terminator") is already done byqualify_min_const_fn
here, so I don't think this additional check is really necessary?Fixing the other, second case in the linked issue was only slightly more involved, since
Vec::new()
is a function call that has the ability to panic, so there must be adrop()
terminator for cleanup, however we should be able to freely ignore that. Const checking ignores cleanup blocks, so we should, too?r? @Jarcho
changelog: [
missing_const_for_fn
]: continue linting on fns with parameters implementingDrop
if they're not actually dropped