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

Simplify code in deserialize_in_place_struct and implement #2387 for in-place case #2443

Merged
merged 8 commits into from Aug 2, 2023

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented May 6, 2023

This PR makes a small refactoring which, I think, improves undertanding of the code that will be generated. During this work I found, that #2387 did not change code generation for in-place case, so the last commit fixes that. The strange thing, that tests does not catch this. I tried to see the generated code using

serde\test_suite> cargo expand --test test_annotations --all-features

and it shows that deserialize_in_place method doesn't generated. I tried

  • add explicit dependency to the serde_derive here:
    [dependencies]
    serde = { path = "../serde" }
  • add explicit requirement for the test:
    [[test]]
    name = "test_annotations"
    required-features = ["serde_derive/deserialize_in_place"]
  • add a new feature deserialize_in_place = ["serde_derive/deserialize_in_place"] here:
    [features]
    unstable = ["serde/unstable"]
  • combine the previous two attempts

Nothing of this is work. So I do not know how to test those changes. I would be appreciate if someone point me out how to correctly test this.

@Mingun
Copy link
Contributor Author

Mingun commented May 21, 2023

Is there something that prevents merging this PR? I now working on proper testcases for enums (there a many pitfalls there) and having simplifications that I made here would be very useful to fix them

@oli-obk
Copy link
Member

oli-obk commented May 22, 2023

The PR mixes functional changes with pure refactorings, requiring a larger amount of contiguous time to review it. I still have 20+ reviews on rustc that are higher priority, so this one keeps getting bumped down. I have no timeline to tell you when I'll be able to review it.

@Mingun
Copy link
Contributor Author

Mingun commented May 22, 2023

I understood. Usually, refactoring is required to see the need of the actual change, and in some cases unavoidable. Most changes are pretty obvious, although. Just review commit-by-commit and follow hints when "ignore whitespace changes" is recommended. I hope you will find time to review it, it shouldn't take more than a half-hour of your time

Copy link
Contributor

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I've had a look at all the commits. They mostly make sense to me but it looks like they are almost all independent of each other. Maybe it would make sense to split this PR into multiple smaller ones that can be independently prioritized / reviewed / discussed / merged?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thank you! This looks great.

Regarding the question in the PR description: I can't tell what might have been going wrong. For me, cargo expand --test test_annotations --all-features correctly generates all the deserialize_in_place functions I would expect (27 of them as of current master). This is fewer than the total number of times derive(Deserialize) is used on a struct in that file, but that's expected because there is a set of conditions where a deserialize_in_place is not supposed to be generated.

test_suite unconditionally enables serde_derive's "deserialize_in_place" feature here:

serde_derive = { path = "../serde_derive", features = ["deserialize_in_place"] }

@dtolnay dtolnay merged commit 83b1a3d into serde-rs:master Aug 2, 2023
16 checks passed
@Mingun Mingun deleted the deserialize-in-place branch August 2, 2023 05:16
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.

None yet

4 participants