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.

Sign up for GitHub

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

Jump to bottom

lib: add try_take! macro, handling AlreadyUsed Options #360

Merged
merged 1 commit into from
Nov 16, 2023
Merged

lib: add try_take! macro, handling AlreadyUsed Options #360

merged 1 commit into from
Nov 16, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Nov 15, 2023 •
edited
Loading

In several places we represent something that could be consumed as an Option<T>. When we try to use it, we take() the option, match the result, and return rustls_result::AlreadyUsed if take() returned None.

Since this pattern is becoming more common with the use of more builder patterns (particularly in #341) this commit adds a try_take! macro that can do this repetitive work for us.

@cpu
Copy link
Member Author

cpu commented Nov 15, 2023

rustls-ffi / Check minimum versions (pull_request) Failing after 34s

Ah, it looks like several CI tasks are failing in this branch (and also in main's scheduled CI runs) due to rust-lang/rust#117693. We fixed this in Rustls w/ rustls/rustls#1582, but that fix is only present in 0.22.0-alpha.4.

I addressed the breakage in #341 with the update to alpha-4 (4f9ecfd) but fixing main will require a Rustls point release in the 0.21.x series. Maybe worthwhile? rustls-ffi probably isn't the only downstream crate that will break from this 🤔

@jsha
Copy link
Collaborator

jsha commented Nov 16, 2023

A point release for rustls 0.21.x would be great. In the meantime I think we can unbreak the rustls-ffi build by pinning a specific nightly rust release rather than the latest nightly. We may also be able to move some of the affected tasks to a stable release.

@cpu cpu self-assigned this Nov 16, 2023
jsha
jsha approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@jsha jsha left a comment

Choose a reason for hiding this comment

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

This is great. One piece of naming feedback: The other try_ macros do non-mutating conversions. This one mutates, specifically calling take(). I think try_take! would be a better name and make it clearer what's happening.

@cpu
lib: add try_take! macro, handling AlreadyUsed Options
8c557c2 
In several places we represent something that could be consumed as an
`Option<T>`. When we try to use it, we `take()` the option, match the
result, and return `rustls_result::AlreadyUsed` if `take()` returned
`None`.

Since this pattern is becoming more common with the use of more builder
patterns this commit adds a `try_take!` macro that can do this
repetitive work for us.
@cpu cpu changed the title lib: add try_consumable! macro, handling AlreadyUsed Options lib: add try_take! macro, handling AlreadyUsed Options Nov 16, 2023
@cpu
Copy link
Member Author

cpu commented Nov 16, 2023

I think try_take! would be a better name and make it clearer what's happening.

Good idea, renamed! The branch name has fallen out of date but I updated everything else. Thanks for the review.

@cpu cpu merged commit 0b35e62 into rustls:main Nov 16, 2023
21 checks passed
@cpu cpu deleted the cpu-try-consumable-macro branch November 16, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsha jsha jsha approved these changes

@ctz ctz ctz approved these changes

Assignees

@cpu cpu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cpu @jsha @ctz