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

Naming convention of as_borrowed #6027

Open
sffc opened this issue Jan 22, 2025 · 6 comments
Open

Naming convention of as_borrowed #6027

sffc opened this issue Jan 22, 2025 · 6 comments
Labels
2.0-breaking Changes that are breaking API changes needs-approval One or more stakeholders need to approve proposal S-small Size: One afternoon (small bug fix or enhancement)

Comments

@sffc
Copy link
Member

sffc commented Jan 22, 2025

Related: #4393

We have a lot of as_borrowed and similar functions through ICU4X.

We should try to be consistent with how these work. Here's what I propose:

  1. as_borrowed should return a Copy wrapper that contains a reference to self. Example signature:
    • impl Foo { pub fn as_borrowed<'a>(&'a self) -> FooBorrowed<'a> {...} }
    • In general, there is one canonical borrowed type corresponding to the current type. as_borrowed should return it.
  2. impl AsRef should return references.
  3. If an impl AsRef is needed in a const context, it can be named as_foo_ref or some other appropriate name, but not as_borrowed since that has meaning prescribed in point 1. It could take the shadow name as_ref if there is only one sensible return value.

Examples of specific impact of this:

  1. Date::wrap_calendar_in_ref() should be Date::as_borrowed()
  2. ZeroTrie::as_borrowed_slice() should be ZeroTrie::as_borrowed()
  3. ZeroTrie::as_borrowed(), which currently returns &ZeroTrie, should be ZeroTrie::as_ref()

But it seems that mostly we already do this. I just found these examples where we are inconsistent.

CC @Manishearth @robertbastian

@sffc sffc added 2.0-breaking Changes that are breaking API changes needs-approval One or more stakeholders need to approve proposal S-small Size: One afternoon (small bug fix or enhancement) labels Jan 22, 2025
@sffc sffc added this to the ICU4X 2.0 Stretch ⟨P2⟩ milestone Jan 22, 2025
@Manishearth
Copy link
Member

  • Date::wrap_calendar_in_ref() should be Date::as_borrowed()

I disagree, this is from a family of functions called wrap_calendar_in. I think it is fine for as-borrowed-style functions being called something more specific if it makes sense in that case.

Agree with the other two.

@sffc
Copy link
Member Author

sffc commented Jan 23, 2025

Should the other Date functions be named as_rc and as_arc?

The whole "wrap calendar in X" is not particularly meaningful to users. They don't care that it's specifically the calendar that gets wrapped. They just care that their Date has a cheap clone afterward.

@Manishearth
Copy link
Member

Hm, perhaps. I'm okay with as_rc + as_arc + as_borrowed.

@robertbastian
Copy link
Member

as_borrowed works because replacing fields by references results in a borrowed version of the struct, but replacing fields by Rcs doesn't result in an "rc" version of the struct, but rather something like an "rced" version.

@robertbastian
Copy link
Member

The whole "wrap calendar in X" is not particularly meaningful to users.

It changes the type from Date<C> to Date<Arc<C>>, I think that's exactly what it says on the tin.

@sffc
Copy link
Member Author

sffc commented Jan 31, 2025

This is a classic "what it does vs why you should use it" type of conflict. "Wrap calendar in Arc" is technically correct, describing "what it does", but it doesn't say anything about why you would want to use it. Most people want this because they want something cloneable without a lifetime. The term "Arc" we can assume to me fairly well known by our clients, so "as_arc" seems maybe about right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-breaking Changes that are breaking API changes needs-approval One or more stakeholders need to approve proposal S-small Size: One afternoon (small bug fix or enhancement)
Projects
None yet
Development

No branches or pull requests

3 participants