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

chore(deps): update #5999

Merged
merged 4 commits into from Jan 13, 2021
Merged

chore(deps): update #5999

merged 4 commits into from Jan 13, 2021

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jan 12, 2021

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

In general this looks good!

I'd like to see us do a little investigation to decide if the large enum variants are OK or if it might be better to box the large ones. Assuming we are ok with it, it'd be nice to leave comments next to the #[allow] blocks about why we are ok with them.

@@ -15,6 +15,7 @@ use std::str::FromStr;

pub use kind::Kind;

#[allow(clippy::large_enum_variant)]
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to add comments for these allows about why we allow them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked regex code for understanding why we actually get this error. So regex use thread_local for some cache. Version 1.1.0 of thread_local was released on Jan 8, 2021 and one of the changes was inline data to structure from the Box:
https://github.com/Amanieu/thread_local-rs/blob/v1.0.1/src/lib.rs#L95
vs
https://github.com/Amanieu/thread_local-rs/blob/v1.1.0/src/lib.rs#L107
Changes were made in this PR: Amanieu/thread_local-rs#22
I'm not sure how this change will affect vector memory consumption, so will revert thread_local update and create a separate issue for this.

@jszwedko
Copy link
Member

Noting that I'm fine opening a follow-up issue for investigating the large enum variants since this PR doesn't change that.

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@jszwedko
Copy link
Member

@fanatid 👍 thanks for investigating!

@fanatid fanatid merged commit 3a9f7e6 into master Jan 13, 2021
@fanatid fanatid deleted the deps-update branch January 13, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: deps Anything related to Vector's dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants