Skip to content

Document type layout of str #410 #441

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

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Document type layout of str #410 #441

merged 1 commit into from
Nov 13, 2018

Conversation

Zaechus
Copy link
Contributor

@Zaechus Zaechus commented Oct 6, 2018

I documented the size behavior of type str to the 9.3.Type Layout section.
Fixes #410

@Zaechus
Copy link
Contributor Author

Zaechus commented Oct 23, 2018

Can someone please review my pull?

@Zaechus
Copy link
Contributor Author

Zaechus commented Oct 23, 2018

Is this any better?

@alercah
Copy link
Contributor

alercah commented Nov 3, 2018

Hm. I think this is ok, but we can do better. Since str is a slice type, I think that mentioning it in the slice section might make more sense, and then call out that it's a UTF-8 representation of its characters. As is, "each character" is kind of vague, and I don't think we need to duplicate the array section which goes into more detail. Really, str is just [u8] with an additional guarantee that it's valid UTF-8.

@alercah
Copy link
Contributor

alercah commented Nov 10, 2018

Can you please rebase this on top of HEAD to remove the merge commits? Otherwise looks good to me.

@Zaechus Zaechus closed this Nov 11, 2018
@Zaechus Zaechus reopened this Nov 11, 2018
@Zaechus Zaechus closed this Nov 11, 2018
@Zaechus Zaechus reopened this Nov 11, 2018
@Zaechus
Copy link
Contributor Author

Zaechus commented Nov 11, 2018

restarting travis

@Zaechus
Copy link
Contributor Author

Zaechus commented Nov 13, 2018

It should be clean now, @alercah .

@alercah alercah merged commit 5e676a7 into rust-lang:master Nov 13, 2018
@alercah
Copy link
Contributor

alercah commented Nov 13, 2018

Thanks <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants