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

ACP: change as_{mut,const} on pointers to cast_{mut,const} #51

Closed
Kixunil opened this issue Jun 16, 2022 · 13 comments
Closed

ACP: change as_{mut,const} on pointers to cast_{mut,const} #51

Kixunil opened this issue Jun 16, 2022 · 13 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@Kixunil
Copy link

Kixunil commented Jun 16, 2022

Proposal

Problem statement

The as_mut method on *const T may be confused with *mut T.

Motivation, use-cases

This confusion already broke the doc once. Also I believe cast_ is more clear, especially since a similar cast() method exists.

Solution sketches

Rename the method to use cast_ prefix

Links and related work

#92675
#98174

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@Kixunil Kixunil added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 16, 2022
@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

This confusion already broke the doc once. Also I believe cast_ is more clear, especially since a similar cast() method exists.

can you link to the example you're referring to?

@cuviper
Copy link
Member

cuviper commented Jun 16, 2022

I'm not sure what broke, but to be more explicit about the confusion, we have one that casts and one that forms a reference, which is an unresolved question on rust-lang/rust#92675.

  • <*const T>::as_mut(self) -> *mut T (unstable)
  • <*mut T>::as_mut<'a>(self) -> Option<&'a mut T> (stable 1.9)

@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

I'm not sure what broke, but to be more explicit about the confusion, we have one that casts and one that forms a reference, which is an unresolved question on rust-lang/rust#92675.

* `<*const T>::as_mut(self) -> *mut T` (unstable)

* `<*mut T>::as_mut<'a>(self) -> Option<&'a mut T>` (stable 1.9)

that seems to support the renaming being proposed here

@scottmcm
Copy link
Member

👍 from me. Having the common prefix sounds good.

@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

Seconded

@rustbot label +initial-comment-period

@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2022

Error: Malformed triagebot.toml in master branch.
missing field zulip_ping for key major-change at line 1 column 1

Please let @rust-lang/release know if you're having trouble with this bot.

@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

gosh dangit

@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

trying to fix it but, uuuh, github is having a moment... #52

just gonna add the label manually for now

@yaahc yaahc added the initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 16, 2022
@Kixunil
Copy link
Author

Kixunil commented Jun 17, 2022

There was also an interesting point that to_mut/to_const may be better: rust-lang/rust#98174 (comment)

Regarding breaking the docs, I meant this: rust-lang/rust#96327

@yaahc
Copy link
Member

yaahc commented Jun 17, 2022

There was also an interesting point that to_mut/to_const may be better: rust-lang/rust#98174 (comment)

According to https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv that wouldn't be consistent with the ways we normally use as_ vs to_. Best alternative I can think up with that is consistent with the naming guidelines would be as_mut_ptr

@cuviper
Copy link
Member

cuviper commented Jun 17, 2022

There is already <*mut [T]>::as_mut_ptr(self) -> *mut T too.

@joboet
Copy link
Member

joboet commented Jul 6, 2022

There was also an interesting point that to_mut/to_const may be better: rust-lang/rust#98174 (comment)

According to https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv that wouldn't be consistent with the ways we normally use as_ vs to_. Best alternative I can think up with that is consistent with the naming guidelines would be as_mut_ptr

Since this is a method of a Copy type which does not change the level of abstraction, to seems more fitting than as when following the API guidelines.

It would put the method in the same category as e.g. f32::to_bits or Cow::to_mut. I think it reads really well.

@joshtriplett
Copy link
Member

We discussed this in today's libs-api meeting, and ended up settling on cast_*.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 22, 2022
…cast, r=scottmcm

Rename `<*{mut,const} T>::as_{const,mut}` to `cast_`

This renames the methods to use the `cast_` prefix instead of `as_` to
make it more readable and avoid confusion with `<*mut T>::as_mut()`
which is `unsafe` and returns a reference.

Sorry, didn't notice ACP process exists, opened rust-lang/libs-team#51

See rust-lang#92675
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 22, 2022
…cast, r=scottmcm

Rename `<*{mut,const} T>::as_{const,mut}` to `cast_`

This renames the methods to use the `cast_` prefix instead of `as_` to
make it more readable and avoid confusion with `<*mut T>::as_mut()`
which is `unsafe` and returns a reference.

Sorry, didn't notice ACP process exists, opened rust-lang/libs-team#51

See rust-lang#92675
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…cottmcm

Rename `<*{mut,const} T>::as_{const,mut}` to `cast_`

This renames the methods to use the `cast_` prefix instead of `as_` to
make it more readable and avoid confusion with `<*mut T>::as_mut()`
which is `unsafe` and returns a reference.

Sorry, didn't notice ACP process exists, opened rust-lang/libs-team#51

See #92675
@dtolnay dtolnay added ACP-accepted API Change Proposal is accepted (seconded with no objections) and removed initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

8 participants