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

Replace try! with ? in serde_derive #2539

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jul 28, 2023

#2366

As of today, this regresses compile time of serde's generated code by 6.5–7.5% which is too bad. However try! has been disastrous for readability of the generated code coming out of cargo expand. I was expecting the performance problems in rustc to be fixed a long time ago but at this point it doesn't seem like that is going to happen.

@dtolnay dtolnay merged commit 7b09ccc into serde-rs:master Jul 28, 2023
21 checks passed
@dtolnay dtolnay deleted the questionmark branch July 28, 2023 02:20
@Mingun
Copy link
Contributor

Mingun commented Jul 28, 2023

I see, that not all try! was replaced by ?. I would say, most of them. Is it so conceived?

@Baptistemontan
Copy link
Contributor

So what about 91ec1c2 ? The crate itself should suffer from readability but it goes backward for generated code ?

@Mingun
Copy link
Contributor

Mingun commented Jul 31, 2023

Yes, that is... very strange decision :)

@dtolnay
Copy link
Member Author

dtolnay commented Aug 12, 2023

It's specifically cargo expand readability that is the motivation.

People are not looking at cargo expand of the serde crate. They are looking at expanded derive macros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants