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

Relax some restrictions on validators #1998

Merged
merged 6 commits into from
Aug 12, 2020
Merged

Relax some restrictions on validators #1998

merged 6 commits into from
Aug 12, 2020

Conversation

CreepySkeleton
Copy link
Contributor

Also relaxes 'static restriction on validators.

Closes #1771

This PR also relaxes the 'static restriction on the validator fn, thus allowing user to use proper closures that capture some context as validators. This should address (and potentially close) #572.

@CreepySkeleton
Copy link
Contributor Author

This PR has grown beyond the simple Send + Sync. New changes:

  • I have made it possible for validators to capture and mutate context. Since we need it to be working along with Send + Sync, validators form now on use atomic synchronization (single mutex acquire-release pair per validator call). I don't think this makes them any slower since acquiring-releasing a mutex is cheap if there's no real competition, and we only ever use one thread.
  • I replaced all the incomprehensible 'b, 'c, 'z... lifetimes with meaningful names. It's really easy to get confused by the lifetimes and use wrong ones when more than one is involved - we have had a few examples of that in the form of &'b Arg<'b> which is wrong.

@CreepySkeleton CreepySkeleton changed the title Make sure that App & Arg & ArgGroup implement Send + Sync Relax some restrictions on validators Jul 3, 2020
@pksunkara
Copy link
Member

@CreepySkeleton Can you please rebase this?

@CreepySkeleton
Copy link
Contributor Author

I think I'll extract lifetime renaming toa separate PR and then rebase this one onto that.

@CreepySkeleton
Copy link
Contributor Author

ping @pksunkara

Pfff, what can I say? Lifetimes in tandem with impl Iterator return types doesn't seem to work very well in some circumstances, see rust-lang/rust#34511 (comment). At any rate, I have rearranged debug assertions a bit. I think the code is more organized and understandable this way.

I have also found a number of wrong tests thanks to the new assert structure (they cover more cases now) which is a point in favor of the rewrite.

Otherwise, I think this is ready and can be merged.

Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 11, 2020

Merge conflict.

@CreepySkeleton
Copy link
Contributor Author

bors r=pksunkara

@bors
Copy link
Contributor

bors bot commented Aug 12, 2020

Build succeeded:

@bors bors bot merged commit 2df656c into master Aug 12, 2020
@bors bors bot deleted the impl_send_sync branch August 12, 2020 14:58
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.

impl Send for App
2 participants