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

Rename MaybeUninit to MaybeUninitialized #56138

Closed
wants to merge 1 commit into from

Conversation

shepmaster
Copy link
Member

@shepmaster shepmaster commented Nov 21, 2018

Proposal to finalize the name as MaybeUninit #56138 (comment) -- as in, the FCP is to close this PR.

rfcbot comment is here


Changed via:

git ls-files -z | xargs -0 -L1 gsed --in-place 's@\bMaybeUninit\b@MaybeUninitialized@'

Checked via:

rg -uuu '\bMaybeUninit\b'

@shepmaster
Copy link
Member Author

r? @RalfJung

@shepmaster
Copy link
Member Author

I know this is racing your other PR, so you get to review!

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member

Fine for me. @rust-lang/libs @rust-lang/lang any objections?

@withoutboats
Copy link
Contributor

What's the motivation for this change?

@shepmaster
Copy link
Member Author

Wrds r btr thn abbrs.

@shepmaster
Copy link
Member Author

For full context, if you start reading the comments here, there's an overwhelming consensus to add more words into the method names of this type to better indicate their unsafety and usage concerns.

Since the point of all of those words is to force the user to think more about careful usage of this type, making sure that the type's name itself is unambiguous is a good start.

@withoutboats
Copy link
Contributor

What could Uninit be ambiguous with?

@withoutboats
Copy link
Contributor

withoutboats commented Nov 21, 2018

The specific example there is into_inner, which is a common name for a safe method that unwraps a newtype into its inner type, which isn't the functionality of this method. Its not about adding more words, its about not using a name that encourages misuse. I don't see a similar motivation here; abbreviations in themselves are quite common in the standard library and not against its style guidelines: Iter, Buf, Rc, Arc, Cow, etc (edit: ptr, mem, fmt, println, ...)

@shepmaster
Copy link
Member Author

Ok

@shepmaster shepmaster closed this Nov 21, 2018
@shepmaster shepmaster deleted the maybe-uninitialized branch November 21, 2018 16:38
@SimonSapin
Copy link
Contributor

There is also a precedent in the standard library to avoid sometimes-well-established abbreviations: copy_from_nonoverlapping, set_permissions, etc.

@RalfJung
Copy link
Member

RalfJung commented Nov 21, 2018

I actually think we should do this and would propose reopening the PR. We usually avoid abbreviations unless they are extremely widely used (like pretty much all of @withoutboats' examples).

Another example is compare_and_swap.

@shepmaster shepmaster restored the maybe-uninitialized branch November 21, 2018 17:13
@shepmaster shepmaster reopened this Nov 21, 2018
@rust-highfive

This comment has been minimized.

@varkor
Copy link
Member

varkor commented Nov 21, 2018

We usually avoid abbreviations unless they are extremely widely used (like pretty much all of @withoutboats' examples).

I think some of the examples like Iter and Buf are not "extremely widely used" (at least compared to Uninit), but are similarly unambiguous. (Personally, I prefer shorter names when unambiguous.)

@RalfJung
Copy link
Member

Note that this replaces mem::uninitialized, so the precedent here clearly is to not abbreviate.

@nikic
Copy link
Contributor

nikic commented Nov 21, 2018

mem::uninit() is ambiguous in that it can abbreviate both uninitialized (a state) and uninitialize (and operation). The ambiguity does not exist with MaybeUnint, which clearly refers to a state.

@joshtriplett
Copy link
Member

I certainly don't want to see this used often, but at the same time, this seems like unnecessary syntactic salt.

@Centril
Copy link
Contributor

Centril commented Nov 22, 2018

My main gripe is with into_inner which is innocent looking and misleading; that's where the actual unsafety lies and that's where I think adjustment is necessary.

I don't think a lot is gained by lengthening the name of the type here but at the same time I don't feel the need for unsafe code to be overly ergonomic to write. So my reaction to this PR is pretty neutral. I'm not for nor against it -- if you can convince everyone else then I'm sold. :)

@WiSaGaN
Copy link
Contributor

WiSaGaN commented Nov 23, 2018

“maybe uninitialized" is pretty commonly used term in system programming: https://stackoverflow.com/questions/14132898/gcc-wuninitialized-wmaybe-uninitialized-issues

System programmers that are used to the term will make more effort to recognize "MaybeUninit" than the more commonly used fully spelled out version. It is also easier to relate "MaybeUninitialized" to the original "mem::uninitialized" method.

@kennytm kennytm added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2018
@bors
Copy link
Contributor

bors commented Nov 27, 2018

☔ The latest upstream changes (presumably #54668) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot

This comment has been minimized.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 14, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Jan 16, 2019

@rfcbot concern i-am-un-maybe-init

@Centril claimed that MaybeUninit is equivalent to MaybeInit. I disagree. Yes, I can see that the two phrases indicate the same underlying semantics: The data here may or may not be initialized. I argue that it is important to stress the "un" in "uninitialized", which justifies the inclusion of those two additional characters. The benefit is to stimulate all those neural pathways associated with "uninitialized" (=> here be dragons).

I'm speaking about the benefit for human authors and code reviewers, especially those who are perhaps looking over something quickly and might not catch an occurrence of "Maybe" (even when next to "Init") but would catch "Uninit".

I don't think there is a net win from renaming replacing MaybeUninit with MaybeInit.


(I don't have an opinion about the original proposal to lengthen the name to MaybeUninitialized. I would have a similar objection to a rename to MaybeInitialized.)

@scottjmaddox
Copy link

pnkfelix's argument seems reasonable to me. There isn't really much of an advantage in renaming to MaybeInit, anyways, other than being ever so slightly easier to type.

@joshtriplett
Copy link
Member

@pnkfelix I agree entirely, and I have the same objection. This not only doesn't seem worth the change, it seems less clear.

@Centril
Copy link
Contributor

Centril commented Jan 21, 2019

@pnkfelix

I'm speaking about the benefit for human authors and code reviewers, especially those who are perhaps looking over something quickly and might not catch an occurrence of "Maybe" (even when next to "Init") but would catch "Uninit".

This is persuasive. Therefore:

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Jan 21, 2019

@Centril proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 21, 2019
@Centril
Copy link
Contributor

Centril commented Jan 21, 2019

I propose that we finalize the name of the type as MaybeUninit (note the Un); The proposal is merely for the type's name and not for stabilization of the type itself.

@rfcbot merge

(EDIT: this is a bit weird.. we're "merging" but this means that the PR itself will be closed...)

@rfcbot
Copy link

rfcbot commented Jan 21, 2019

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 21, 2019
@shepmaster
Copy link
Member Author

shepmaster commented Jan 22, 2019

especially those who are perhaps looking over something quickly and might not catch an occurrence of "Maybe" (even when next to "Init") but would catch "Uninit".

I don't know enough HCI to refute this claim, but it seems odd to expect people to read the middle of a word but not the beginning. Similarly, it seems odd that people are more likely to catch "Uninit" than they would be "Uninitialized".

@nikomatsakis
Copy link
Contributor

While I checked my name, I would like to note for the record that I prefer the name Uninit -- I feel the maybe is implied. But I don't really care enough to argue strongly one way or the other. =)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 4, 2019
@rfcbot
Copy link

rfcbot commented Feb 4, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@tesuji
Copy link
Contributor

tesuji commented Feb 8, 2019

I am not a native English speaker, and have not used much unsafe code. So every time I see MaybeUninit, I read it like maybe uni-nit. That's so weird!

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 12, 2019

I find the mixture of abbreviations with long spellings in the API weird: MaybeUninit vs ::into_initialized. If we want to keep the abbreviation in the name (use MaybeUninit or Uninit - I prefer Uninit since the maybe is implied), then it might be worth it to also use the abbreviation in the other apis, like ::into_init(), for consistency.

Without consistency, unless I use this API all-day every-day I pretty much have to guess whenever I need it. I wish the standard library would have clear guidelines about this instead of each API doing its own thing, it would save us a lot of time (bikeshedding, context-switching to the docs because rustc does not suggest the right thing, etc.).

@rfcbot
Copy link

rfcbot commented Feb 14, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 14, 2019
@RalfJung
Copy link
Member

So, uh, I guess we just close this PR now?

@Centril
Copy link
Contributor

Centril commented Feb 14, 2019

Indeed we should; Thank you @shepmaster for your work & proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.