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.

Sign up for GitHub

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

Jump to bottom

work on char/str descriptions #809

Merged
merged 5 commits into from
May 14, 2020
Merged

work on char/str descriptions #809

merged 5 commits into from
May 14, 2020

Conversation

RalfJung
Copy link
Member

Cc @ehuss (#792)

ehuss
ehuss reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this!

src/types/textual.md Outdated Show resolved Hide resolved
src/types/textual.md Outdated Show resolved Hide resolved
RalfJung and others added 2 commits May 12, 2020 21:40
@RalfJung @ehuss
fix link
3c6a8f4 
Co-authored-by: Eric Huss <eric@huss.org>
@RalfJung
clarify representation
bd74860
Havvy
Havvy reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather the description of str be what the language definition considers and then heavily point out the library invariant afterwards instead of giving the library invariant definition and then noting that the language is technically more lax. That said, the content is correct enough that I'm not going to mark "request changes".

@RalfJung
Copy link
Member Author

@Havvy what about this?

Havvy
Havvy reviewed May 13, 2020
View reviewed changes
src/types/textual.md Outdated
8-bit unsigned bytes. However, the Rust standard library makes extra assumptions
about `str`: methods working on `str` assume and ensure that the data in there
is valid UTF-8. Calling a `str` method with a non-UTF-8 buffer can cause
[Undefined Behavior] now or in the future. \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the trailing backslash here on purpose? I can't remember what markdown does to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a newline without starting a new paragraph. I found like the next sentence is somewhat disconnected from the previous ones. You can see that in action by clicking the "view file" link in the PR diff view.

I can remove it if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, as long as it has a purpose, it looks fine to me. I recall there being discussion of changing to one sentence = one line anyways, although it never went anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall there being discussion of changing to one sentence = one line anyways

That's different, that's for just the markdown file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see on, uh, the third look at that file. It's subtle, and I don't see a difference in readability either way personally, but if you see it, I'm willing to trust a second set of eyes.

Havvy
Havvy approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as that backslash is necessary or harmless.

@Havvy Havvy requested a review from ehuss May 13, 2020 18:59
ehuss
ehuss approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moving the DST point to a separate paragraph. We don't use newlines for formatting anywhere else, and I don't think there's really a good reason to do it here.

@ehuss ehuss merged commit 7a60492 into rust-lang:master May 14, 2020
@RalfJung RalfJung deleted the str-invariant branch May 14, 2020 16:21
@ehuss ehuss mentioned this pull request May 26, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 26, 2020
@Dylan-DPC
Rollup merge of rust-lang#72592 - ehuss:update-books, r=ehuss
6656a6d 
Update books

## reference

7 commits in 892b928b565e35d25b6f9c47faee03b94bc41489..becdca9477c9eafa96a4eea5156fe7a2730d9dd2
2020-05-11 11:13:51 -0700 to 2020-05-21 21:08:02 +0100
- Update tuple index token. (rust-lang/reference#814)
- Fixes minor errors (rust-lang/reference#818)
- Update that macros can be deprecated. (rust-lang/reference#813)
- work on char/str descriptions (rust-lang/reference#809)
- cfg_attr needs a valid predicate (rust-lang/reference#812)
- Account for removal of UB in float-to-int casts (rust-lang/reference#810)
- Fix stray plus signs. (rust-lang/reference#811)

## book

6 commits in 6247be15a7f7509559f7981ee2209b9e0cc121df..e8a4714a9d8a6136a59b8e63544e149683876e36
2020-05-03 10:55:09 -0500 to 2020-05-25 10:29:27 -0500
- code is 1024 now, not 512
- Clean up install a bit
- We don't need build.sh anymore
- Fix CI status in README
- Port to github actions (rust-lang/book#2337)
- operating system -&gt; allocator

## rust-by-example

5 commits in ab072b14393cbd9e8a1d1d75879bf51e27217bbb..7aa82129aa23e7e181efbeb8da03a2a897ef6afc
2020-05-09 08:46:39 -0300 to 2020-05-25 14:54:26 -0300
- Person of age 0 is alive (rust-lang/rust-by-example#1348)
- Gramatical fix in std/rc.md (rust-lang/rust-by-example#1347)
- Capture example should use String (rust-lang/rust-by-example#1331)
- Fix empty bound examples (rust-lang/rust-by-example#1343)
- Fix an inline comment in macros/repeat.md (rust-lang/rust-by-example#1344)

## edition-guide

1 commits in 49270740c7a4bff2763e6bc730b191d45b7d5167..0a8ab5046829733eb03df0738c4fafaa9b36b348
2020-05-11 08:50:29 -0500 to 2020-05-18 08:34:23 -0500
- Changes for Rust 1.32 & setup for edition-next (rust-lang/edition-guide#213)

## embedded-book

3 commits in 366c50a03bed928589771eba8a6f18e0c0c01d23..5555a97f04ad7974ac6fb8fb47c267c4274adf4a
2020-05-07 09:04:42 +0000 to 2020-05-25 18:00:51 +0000
- Remove reference to const-fn feature of cortex-m. Closes rust-embedded/book#242.  (rust-embedded/book#243)
- Spelling: Appplication -&gt; Application  (rust-embedded/book#241)
- QEMU debugging updates  (rust-embedded/book#239)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 26, 2020
@Dylan-DPC
Rollup merge of rust-lang#72592 - ehuss:update-books, r=ehuss
e061c40 
Update books

## reference

7 commits in 892b928b565e35d25b6f9c47faee03b94bc41489..becdca9477c9eafa96a4eea5156fe7a2730d9dd2
2020-05-11 11:13:51 -0700 to 2020-05-21 21:08:02 +0100
- Update tuple index token. (rust-lang/reference#814)
- Fixes minor errors (rust-lang/reference#818)
- Update that macros can be deprecated. (rust-lang/reference#813)
- work on char/str descriptions (rust-lang/reference#809)
- cfg_attr needs a valid predicate (rust-lang/reference#812)
- Account for removal of UB in float-to-int casts (rust-lang/reference#810)
- Fix stray plus signs. (rust-lang/reference#811)

## book

6 commits in 6247be15a7f7509559f7981ee2209b9e0cc121df..e8a4714a9d8a6136a59b8e63544e149683876e36
2020-05-03 10:55:09 -0500 to 2020-05-25 10:29:27 -0500
- code is 1024 now, not 512
- Clean up install a bit
- We don't need build.sh anymore
- Fix CI status in README
- Port to github actions (rust-lang/book#2337)
- operating system -&gt; allocator

## rust-by-example

5 commits in ab072b14393cbd9e8a1d1d75879bf51e27217bbb..7aa82129aa23e7e181efbeb8da03a2a897ef6afc
2020-05-09 08:46:39 -0300 to 2020-05-25 14:54:26 -0300
- Person of age 0 is alive (rust-lang/rust-by-example#1348)
- Gramatical fix in std/rc.md (rust-lang/rust-by-example#1347)
- Capture example should use String (rust-lang/rust-by-example#1331)
- Fix empty bound examples (rust-lang/rust-by-example#1343)
- Fix an inline comment in macros/repeat.md (rust-lang/rust-by-example#1344)

## edition-guide

1 commits in 49270740c7a4bff2763e6bc730b191d45b7d5167..0a8ab5046829733eb03df0738c4fafaa9b36b348
2020-05-11 08:50:29 -0500 to 2020-05-18 08:34:23 -0500
- Changes for Rust 1.32 & setup for edition-next (rust-lang/edition-guide#213)

## embedded-book

3 commits in 366c50a03bed928589771eba8a6f18e0c0c01d23..5555a97f04ad7974ac6fb8fb47c267c4274adf4a
2020-05-07 09:04:42 +0000 to 2020-05-25 18:00:51 +0000
- Remove reference to const-fn feature of cortex-m. Closes rust-embedded/book#242.  (rust-embedded/book#243)
- Spelling: Appplication -&gt; Application  (rust-embedded/book#241)
- QEMU debugging updates  (rust-embedded/book#239)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Havvy Havvy Havvy approved these changes

@ehuss ehuss ehuss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RalfJung @ehuss @Havvy