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

Decide on the fate of the Borrowed variant in DataPayload #752

Closed
sffc opened this issue Jun 3, 2021 · 5 comments · Fixed by #904
Closed

Decide on the fate of the Borrowed variant in DataPayload #752

sffc opened this issue Jun 3, 2021 · 5 comments · Fixed by #904
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Milestone

Comments

@sffc
Copy link
Member

sffc commented Jun 3, 2021

The Borrowed variant of DataPayload[Inner] pulls in the 'd lifetime parameter. This has caused some challenges, including:

  • Less ergonomic (extra lifetime parameter)
  • Issues related to trait objects (see comment in erased.rs)

The main use case for the Borrowed variant is to pave the way toward future no-alloc use cases.

If we decided to keep the Borrowed variant, there are some options we could explore to work around the trait object issues mentioned above.

Also decide on the names of the lifetimes ('d and 's).

@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Jun 3, 2021
@sffc
Copy link
Member Author

sffc commented Jun 4, 2021

2021-06-04: Conclusion: Removed the Borrowed variant, and add it back in the future when we have a clearer use case.

@sffc
Copy link
Member Author

sffc commented Jun 4, 2021

  • @echeran - I think 'cart and 'data are short and descriptive
  • @Manishearth - I think the cart should not be something most users worry about
  • @sffc - The issue with 'data is that it's unclear what happens if we need to add a second lifetime
  • @Manishearth - Okay, that's convincing

Conclusion: Eliminate 'd and replace 's with 'cart

@sffc sffc self-assigned this Jun 4, 2021
@sffc sffc added 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 and removed discuss Discuss at a future ICU4X-SC meeting labels Jun 4, 2021
@sffc sffc added this to the ICU4X 0.3 milestone Jun 4, 2021
@sffc sffc added the discuss-priority Discuss at the next ICU4X meeting label Jun 9, 2021
@sffc
Copy link
Member Author

sffc commented Jun 9, 2021

Some more ideas for the lifetime name:

  • 'ds for "data struct"
  • 'st short for "string" or "struct"
  • 'ld for "locale data"
  • 'locd for "locale data"
  • 'icudata
  • 'data

@sffc
Copy link
Member Author

sffc commented Jun 10, 2021

2021-06-10: Go with 'data since it is more clear when reading code.

@sffc sffc removed the discuss-priority Discuss at the next ICU4X meeting label Jun 10, 2021
@sffc sffc modified the milestones: ICU4X 0.3, 2021 Q3-m1 Jul 13, 2021
@sffc
Copy link
Member Author

sffc commented Jul 21, 2021

Check if impl_dyn_clone is still needed.

@sffc sffc added S-medium Size: Less than a week (larger bug fix or enhancement) and removed S-small Size: One afternoon (small bug fix or enhancement) labels Jul 26, 2021
@sffc sffc closed this as completed in #904 Jul 27, 2021
@sffc sffc modified the milestones: 2021 Q3-m1, ICU4X 0.3 Jul 29, 2021
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-medium Size: Less than a week (larger 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.

1 participant