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

Fixing Option borrows (follow-up #1550) #1556

Merged
merged 7 commits into from
Jan 31, 2022

Conversation

robertbastian
Copy link
Member

No description provided.

Manishearth
Manishearth previously approved these changes Jan 28, 2022
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

lgtm but i'd like to wait for shane's review too

feature = "provider_serde",
serde(borrow, with = "serde_with::As::<[serde_with::BorrowCow; 12]>")
)]
pub [Cow<'data, str>; 12],
Copy link
Member

Choose a reason for hiding this comment

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

question: is this necessary for [Cow; N] as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

}
);

#[cfg(all(test, feature = "provider_serde"))]
mod test {
Copy link
Member

Choose a reason for hiding this comment

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

thought: a cool test to perform (not in this PR) would be to use rust's custom per-collection allocator support to write a wrapping allocator that keeps track of memory allocated, and then construct a postcard based fs/static data provider and deserialize absolutely everything, printing out how much heap memory each component takes.

@Manishearth Manishearth mentioned this pull request Jan 28, 2022
8 tasks
sffc
sffc previously approved these changes Jan 28, 2022
Copy link
Member

@sffc sffc 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 for fixing this!

components/datetime/Cargo.toml Outdated Show resolved Hide resolved
@robertbastian robertbastian dismissed stale reviews from sffc and Manishearth via a69941c January 28, 2022 22:28
@robertbastian
Copy link
Member Author

I'll need to come back to [Cow<str>; N] next week

utils/serde_utils/Cargo.toml Outdated Show resolved Hide resolved
@Manishearth Manishearth mentioned this pull request Jan 28, 2022
Manishearth
Manishearth previously approved these changes Jan 31, 2022
sffc
sffc previously approved these changes Jan 31, 2022
Comment on lines +11 to +12
// Cows fail to borrow in some situations (array, option), but structs of Cows don't.
pub struct CowWrap<'data>(#[serde(borrow)] Cow<'data, str>);
Copy link
Member

Choose a reason for hiding this comment

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

Praise: Nice solution!

provider/core/src/serde/borrow_de_utils.rs Outdated Show resolved Hide resolved
Co-authored-by: Shane F. Carr <shane@unicode.org>
@sffc
Copy link
Member

sffc commented Jun 5, 2024

We're still using this workaround and it's still causing us to waste engineering time:

#4706
#2822
#1550

(future issues will link to here so we can continue to track the impact of this bug)

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.

3 participants