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

fix: don't replace default members' body #12832

Merged
merged 1 commit into from
Jul 24, 2022

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Jul 20, 2022

cc #12779, #12821
addresses #12821 (comment)

gen_trait_fn_body() only attempts to implement required trait member functions, so we shouldn't call it for Implement default members assist.

This patch also documents the precondition of gen_trait_fn_body() and inserts debug_assert!, but I'm not entirely sure if the assertions are appropriate.

@Veykril
Copy link
Member

Veykril commented Jul 24, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 24, 2022

📌 Commit 7e0bd37 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 24, 2022

⌛ Testing commit 7e0bd37 with merge 894d6a2...

@bors
Copy link
Contributor

bors commented Jul 24, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 894d6a2 to master...

@bors bors merged commit 894d6a2 into rust-lang:master Jul 24, 2022
@lnicola
Copy link
Member

lnicola commented Jul 25, 2022

but I'm not entirely sure if the assertions are appropriate.

I think we'd generally use stdx::always! here.

@lowr
Copy link
Contributor Author

lowr commented Jul 25, 2022

I'll keep that in mind, thanks! Is it worth another PR to replace the assertions?

@lnicola
Copy link
Member

lnicola commented Jul 25, 2022

I think so, at least for consistency (just in case another contributor wants to do something the same way), but I can do it later today if you don't get a chance bu then.

bors added a commit that referenced this pull request Jul 25, 2022
Replace `debug_assert!` with `stdx::always!`

Addresses #12832 (comment)
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.

4 participants