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

Change the type of AssertModuleSource::available_cgus. #75210

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Aug 6, 2020

It's currently a BTreeSet<Symbol>, which is a strange type. The
BTreeSet suggests that element order is important, but Symbol is a
type whose ordering isn't useful to humans. The ordering of the
collection only manifests in an obscure error message ("no module named
...") that doesn't appear in any tests.

This commit changes the Symbol to a String, which is more
typical.

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2020
@malbarbo
Copy link
Contributor

malbarbo commented Aug 6, 2020

Does it affect the build reproducibility?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Aug 6, 2020

#65657 changed the InternedString here to a Symbol, which would have changed the output order of the error message. Incremental tests aren't blessed, so this didn't cause any failures, but presumably it's nice to have modules be in some sane order when debugging. It should have become a SymbolStr or preferably just String, since SymbolStr is not really sound when put on the heap and performance doesn't matter here.

It's currently a `BTreeSet<Symbol>`, which is a strange type. The
`BTreeSet` suggests that element order is important, but `Symbol` is a
type whose ordering isn't useful to humans. The ordering of the
collection only manifests in an obscure error message ("no module named
`...`") that doesn't appear in any tests.

This commit changes the `Symbol` to a `String`, which is more
typical.
@nnethercote nnethercote force-pushed the change-type-of-available_cgus branch from f1aa8af to ebbf07a Compare August 6, 2020 20:58
@nnethercote
Copy link
Contributor Author

I have changed it to a BTreeSet<String>.

@ecstatic-morse
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2020

📌 Commit ebbf07a has been approved by ecstatic-morse

@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 Aug 6, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#74888 (compiletest: ignore-endian-big, fixes rust-lang#74829, fixes rust-lang#74885)
 - rust-lang#75175 (Make doctests of Ipv4Addr::from(u32) easier to read)
 - rust-lang#75179 (Remove unused FromInner impl for Ipv4Addr)
 - rust-lang#75181 (Fix typo in  `librustc_feature/active.rs`)
 - rust-lang#75183 (Label rustfmt toolstate issues with A-rustfmt)
 - rust-lang#75188 (Handle fieldless tuple structs in diagnostic code)
 - rust-lang#75190 (Clean up E0746 explanation)
 - rust-lang#75210 (Change the type of `AssertModuleSource::available_cgus`.)
 - rust-lang#75211 (Note about endianness of returned value of {integer}::from_be_bytes and friends)
 - rust-lang#75217 (Clean up E0747 explanation)
 - rust-lang#75232 (Fix typo "TraitObligatiom" -> "TraitObligation")
 - rust-lang#75236 (Fix typo "biset" -> "bitset")

Failed merges:

r? @ghost
@bors bors merged commit 19d4e1d into rust-lang:master Aug 7, 2020
@nnethercote nnethercote deleted the change-type-of-available_cgus branch August 7, 2020 04:58
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants