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

Consider passing DataRequest, and other arguments, by value #1077

Closed
sffc opened this issue Sep 22, 2021 · 7 comments · Fixed by #1416
Closed

Consider passing DataRequest, and other arguments, by value #1077

sffc opened this issue Sep 22, 2021 · 7 comments · Fixed by #1416
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt

Comments

@sffc
Copy link
Member

sffc commented Sep 22, 2021

Original issue: There are several places where it would be useful for a DataProvider impl to have an owned DataRequest. It isn't clear why we're passing it by shared reference.

Scope increase: In general, what should be our style for when to take things by value versus by reference? DataRequest is an example of something we may as well take by value, but it's not the only one: options bags are borderline, as are locales, etc.

@sffc sffc added C-data-infra Component: provider, datagen, fallback, adapters v1 T-techdebt Type: ICU4X code health and tech debt S-small Size: One afternoon (small bug fix or enhancement) labels Sep 22, 2021
@sffc sffc self-assigned this Sep 30, 2021
@sffc sffc added this to the ICU4X 0.5 milestone Sep 30, 2021
@sffc sffc modified the milestones: ICU4X 0.5, 2021 Q4 0.5 Sprint A Oct 21, 2021
@sffc sffc added the needs-approval One or more stakeholders need to approve proposal label Nov 18, 2021
@sffc
Copy link
Member Author

sffc commented Nov 18, 2021

Do you agree with changing the DataProvider trait to take DataRequest by value?

@Manishearth
Copy link
Member

Seems fine, the request should 99% of the time be cheap to clone anyway

@sffc sffc added needs-approval One or more stakeholders need to approve proposal and removed needs-approval One or more stakeholders need to approve proposal labels Nov 21, 2021
@sffc
Copy link
Member Author

sffc commented Nov 22, 2021

I'm scope-creeping this issue to include options bags and other arguments. I think we haven't had a deep discussion about when to pass by reference and when to pass by value.

Need updated input from:

@sffc sffc changed the title Consider passing DataRequest by value Consider passing DataRequest, and other arguments, by value Nov 22, 2021
@Manishearth
Copy link
Member

Manishearth commented Nov 22, 2021

Options bags given at Formatter construction time, right? I think those should be by value, but if we ever pass additional options during formatting that should ideally be by reference.

In particular: constructing a formatter is a "one time" operation, it should be fine to take ownership, and it makes sense for the formatter to need ownership of these things. It's a long lived object.

However when a formatter is formatting something all of the parameters should be by-borrow or by-copy since that's a much more ephemeral operation.

I feel like this matches the needs of Formatter objects anyway: options bags at creation would need to be owned, but additional options at format-time can just be read.

(I don't think we really have "additional options at format time" anyway, though one could call the precision bits of FixedDecimal to be "additional options")

@sffc
Copy link
Member Author

sffc commented Dec 9, 2021

I want to use this issue for DataRequest, and spawn a new issue for the options bags and other arguments that we should evaluate in Q1.

@dminor
Copy link
Contributor

dminor commented Dec 9, 2021

I agree with Manish.

@gregtatum
Copy link
Member

Manish's options bag analysis seems reasonable to me.

@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Feb 17, 2022
@sffc sffc removed the v1 label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants