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

Add AnyProvider #1495

Merged
merged 37 commits into from
Jan 14, 2022
Merged

Add AnyProvider #1495

merged 37 commits into from
Jan 14, 2022

Conversation

sffc
Copy link
Member

@sffc sffc commented Jan 11, 2022

Fixes #1479

This is a work-in-progress. Still to-do:

  • Tests and documentation
  • Find and replace all uses of the old ErasedDataProvider with AnyProvider
  • Delete ErasedDataProvider

@sffc sffc requested a review from Manishearth January 11, 2022 09:24
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • provider/core/src/data_provider.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member Author

sffc commented Jan 12, 2022

^ this is a rebase in order to pull in the new ZCF

@sffc sffc marked this pull request as ready for review January 13, 2022 07:18
@sffc
Copy link
Member Author

sffc commented Jan 13, 2022

The commit history is a big mess, but the final result should be fairly clean.

I removed erased.rs and replaced it with any.rs. The two files ended up being more similar than I had anticipated, but AnyProvider is more up-to-date with our latest design.

I also rewrote StructProvider to operate on AnyPayloads. This makes it much more flexible, as can be seen in the DateTimeFormat test refactoring included in this PR.

It may be desirable to rename the struct_provider module now that there is no type named StructProvider any more.

@sffc
Copy link
Member Author

sffc commented Jan 13, 2022

The types I ended up with are as follows:

  • AnyProvider is the main trait. It has a method that returns AnyResponse
  • AnyPayload has the two variants discussed in the issue.
  • DataProvider<AnyMarker> works, and it is used by impl_dyn_provider!(). I could have broken impl_dyn_provider!() but I saw no reason to. To convert from DataProvider<AnyMarker> to AnyProvider, use the .as_any_provider() function.

Manishearth
Manishearth previously approved these changes Jan 13, 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.

This looks great and is a much smaller change than I anticipated! A bunch of thoughts and questions, but nothing blocking, r=me

components/datetime/tests/datetime.rs Show resolved Hide resolved
ffi/diplomat/src/decimal.rs Show resolved Hide resolved
provider/core/src/any.rs Show resolved Hide resolved
provider/core/src/any.rs Show resolved Hide resolved
for<'a> YokeTraitHack<<M::Yokeable as Yokeable<'a>>::Output>: Clone,
M::Yokeable: ZeroCopyFrom<'static, M::Yokeable>,
{
self.try_unwrap_owned()?.downcast()
Copy link
Member

Choose a reason for hiding this comment

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

question: are we ever in danger from users manually constructing non-owned anypayloads?

(i know the current code has this too)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test to demonstrate how one could trigger this code path to run.

provider/core/src/fork/by_key.rs Outdated Show resolved Hide resolved
pub key: ResourceKey,
pub data: DataPayload<M>,
pub data: AnyPayload,
Copy link
Member

Choose a reason for hiding this comment

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

thought(nb): These fields being public are fine but it might be nice API-wise to have a convenient constructor

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pre-existing issue, and I may make further revisions to this class after I do the other issues I have lined up, so I would rather not add a constructor right now.

@sffc sffc merged commit f13e094 into unicode-org:main Jan 14, 2022
@sffc sffc deleted the anyprovider branch January 14, 2022 03:59
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.

Make AnyProvider / ErasedDataProvider be zero-alloc and cloneable
2 participants