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

Improve the readability of List<T>. #91617

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Improve the readability of List<T>. #91617

merged 1 commit into from
Dec 11, 2021

Conversation

nnethercote
Copy link
Contributor

This commit does the following.

  • Expands on some of the things already mentioned in comments.
  • Describes the uniqueness assumption, which is critical but wasn't
    mentioned at all.
  • Rewrites empty() into a clearer form, as provided by Daniel
    Henry-Mantilla on Zulip.
  • Reorders things slightly so that more important things
    are higher up, and incidental things are lower down, which makes
    reading the code easier.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2021
danielhenrymantilla
danielhenrymantilla approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, I like very much the fact that it points out that interning is (expected) to be provided at an outer layer, even though this type expects such semantics 👌

compiler/rustc_middle/src/ty/list.rs
data: [T; 0],

opaque: OpaqueListContents,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To expand on what I mentioned on Zulip, note here that data and opaque are deeply intertwined; having them laid out in such a bare fashion is thus an itsy bit error-prone. That is, compare the current definition, even with your great comments added to it, to:

#[repr(C)]
pub struct List<T> {
    len: usize,

    /// `len` elements are expected to be present.
    data: ThinDst<[T]>,
}

/// Const-assert that a pointer to a `List<T>` is thin.
const _: () = {
    let _ = ::core::mem::transmute::<&List<u8>, usize>;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you happy with this part of the code as is, or are you suggesting that I change it to what's in the Playground link?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't claim which one is better in a vacuum: I like my suggestion since it reads quite well at the level of the definition of List<T>, but now it's the definition of ThinDst which may seem a bit too "messy" for compiler code. So just let it be, unless other people were to chime in in defense of the ThinDst helper

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/list.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

Thanks for the good comments, I have addressed them and updated the code.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
lcnr
lcnr approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

some additional small nits, deal with them as you like.

after that r=me

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/list.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/list.rs Show resolved Hide resolved
@nnethercote
Improve the readability of List<T>.
769a707 
This commit does the following.
- Expands on some of the things already mentioned in comments.
- Describes the uniqueness assumption, which is critical but wasn't
  mentioned at all.
- Rewrites `empty()` into a clearer form, as provided by Daniel
  Henry-Mantilla on Zulip.
- Reorders things slightly so that more important things
  are higher up, and incidental things are lower down, which makes
  reading the code easier.
@nnethercote
Copy link
Contributor Author

nnethercote commented Dec 9, 2021 •
edited
Loading

New code is up. Gotta be close to an r+ by now 😀

@lcnr
Copy link
Contributor

lcnr commented Dec 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2021

📌 Commit 769a707 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2021
@matthiaskrgr
Rollup merge of rust-lang#91617 - nnethercote:improve-List-readabilit…
d06376e 
…y, r=lcnr

Improve the readability of `List<T>`.

This commit does the following.
- Expands on some of the things already mentioned in comments.
- Describes the uniqueness assumption, which is critical but wasn't
  mentioned at all.
- Rewrites `empty()` into a clearer form, as provided by Daniel
  Henry-Mantilla on Zulip.
- Reorders things slightly so that more important things
  are higher up, and incidental things are lower down, which makes
  reading the code easier.

r? `@lcnr`
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 9, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
@matthiaskrgr
Rollup merge of rust-lang#91617 - nnethercote:improve-List-readabilit…
a6a38c2 
…y, r=lcnr

Improve the readability of `List<T>`.

This commit does the following.
- Expands on some of the things already mentioned in comments.
- Describes the uniqueness assumption, which is critical but wasn't
  mentioned at all.
- Rewrites `empty()` into a clearer form, as provided by Daniel
  Henry-Mantilla on Zulip.
- Reorders things slightly so that more important things
  are higher up, and incidental things are lower down, which makes
  reading the code easier.

r? ``@lcnr``
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 10, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
@matthiaskrgr
Rollup merge of rust-lang#91617 - nnethercote:improve-List-readabilit…
7dea631 
…y, r=lcnr

Improve the readability of `List<T>`.

This commit does the following.
- Expands on some of the things already mentioned in comments.
- Describes the uniqueness assumption, which is critical but wasn't
  mentioned at all.
- Rewrites `empty()` into a clearer form, as provided by Daniel
  Henry-Mantilla on Zulip.
- Reorders things slightly so that more important things
  are higher up, and incidental things are lower down, which makes
  reading the code easier.

r? ```@lcnr```
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
@bors
Auto merge of rust-lang#91792 - matthiaskrgr:rollup-2epg7jq, r=matthi…
b8dc6aa 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91617 (Improve the readability of `List<T>`.)
 - rust-lang#91640 (Simplify collection of in-band lifetimes)
 - rust-lang#91682 (rustdoc: Show type layout for type aliases)
 - rust-lang#91711 (Improve `std::iter::zip` example)
 - rust-lang#91717 (Add deprecation warning for --passes)
 - rust-lang#91718 (give more help in the unaligned_references lint)
 - rust-lang#91782 (Correct since attribute for `is_symlink` feature)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1de7815 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@nnethercote nnethercote mentioned this pull request Feb 4, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhenrymantilla danielhenrymantilla danielhenrymantilla approved these changes

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rust-log-analyzer @lcnr @bors @danielhenrymantilla @apiraino @rust-highfive @rustbot