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

Take substs into account in conservative_is_privately_uninhabited #62261

Closed
wants to merge 1 commit into from
Closed

Take substs into account in conservative_is_privately_uninhabited #62261

wants to merge 1 commit into from

Conversation

varkor
Copy link
Member

@varkor varkor commented Jun 30, 2019

No description provided.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Jun 30, 2019
@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

Some before/after tests would be great :)

@varkor
Copy link
Member Author

varkor commented Jul 1, 2019

conservative_is_privately_uninhabited is mainly used for sanity checks that don't affect code output. I think it's only used functionally to determine the layout of an uninhabited array. Maybe it'd be possible to test the codegen of uninhabited arrays; I'll add one when I get the time. However, this is an optimisation improvement in an edge case rather than a correctness fix, so I don't think it's critical (that is, it could be left as an E-easy follow up).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 1, 2019

I can't review this properly until the 18th

@Mark-Simulacrum Mark-Simulacrum added S-inactive and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2019 •
edited
Loading

So... I'm guessing that this changes the layout's Abi of [Struct<!>; 1] to uninhabited, where before it was inhabited? (for struct Struct<T>(T, i32);. I'm not sure how to write a test for that

@varkor
Copy link
Member Author

varkor commented Jul 14, 2019

I'm guessing that this changes the layout's Abi of [Struct<!>; 1] to uninhabited, where before it was inhabited?

Yes, that's right.

@varkor
Copy link
Member Author

varkor commented Jul 22, 2019

I think it's reasonable not to test this. It's deliberately conservative, and either behaviour would be acceptable.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 23, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jul 23, 2019

📌 Commit 962bf69 has been approved by oli-obk

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 23, 2019
@Centril
Rollup merge of rust-lang#62261 - varkor:conservative_is_privately_un…
57d2b28 
…inhabited-subst, r=oli-obk

Take substs into account in `conservative_is_privately_uninhabited`
@Centril Centril mentioned this pull request Jul 23, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
@Centril
Rollup merge of rust-lang#62261 - varkor:conservative_is_privately_un…
6afb4e8 
…inhabited-subst, r=oli-obk

Take substs into account in `conservative_is_privately_uninhabited`
@Centril Centril mentioned this pull request Jul 24, 2019
Closed
bors added a commit that referenced this pull request Jul 24, 2019
@bors
Auto merge of #62923 - Centril:rollup-53i3am3, r=Centril
486833d 
Rollup of 11 pull requests

Successful merges:

 - #62261 (Take substs into account in `conservative_is_privately_uninhabited`)
 - #62528 (Add joining slices of slices with a slice separator, not just a single item)
 - #62738 (Remove uses of mem::uninitialized from std::sys::cloudabi)
 - #62784 (Add riscv32i-unknown-none-elf target)
 - #62808 (Revert "Disable stack probing for gnux32.")
 - #62814 (add support for hexagon-unknown-linux-musl)
 - #62822 (Improve some pointer-related documentation)
 - #62890 (Normalize use of backticks in compiler messages for libsyntax/*)
 - #62901 (cleanup: Remove `extern crate serialize as rustc_serialize`s)
 - #62905 (Normalize use of backticks in compiler messages for doc)
 - #62908 (normalize use of backticks for compiler messages in remaining modules)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
@Centril
Rollup merge of rust-lang#62261 - varkor:conservative_is_privately_un…
cf1e51b 
…inhabited-subst, r=oli-obk

Take substs into account in `conservative_is_privately_uninhabited`
@Centril Centril mentioned this pull request Jul 24, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
@Centril
Rollup merge of rust-lang#62936 - Centril:rollup-d702q3x, r=Centril
8f5f607 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#60938 (rustdoc: make #[doc(include)] relative to the containing file)
 - rust-lang#61890 (Fix some sanity checks)
 - rust-lang#62261 (Take substs into account in `conservative_is_privately_uninhabited`)
 - rust-lang#62528 (Add joining slices of slices with a slice separator, not just a single item)
 - rust-lang#62735 (Turn `#[global_allocator]` into a regular attribute macro)
 - rust-lang#62801 (Remove support for -Zlower-128bit-ops)
 - rust-lang#62808 (Revert "Disable stack probing for gnux32.")
 - rust-lang#62819 (Display name of crate requiring rustc_private in error messages.)
 - rust-lang#62904 (Disable d32 on armv6 hf targets)
 - rust-lang#62907 (Initialize the MSP430 AsmParser)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
@Centril
Rollup merge of rust-lang#62261 - varkor:conservative_is_privately_un…
c82c77d 
…inhabited-subst, r=oli-obk

Take substs into account in `conservative_is_privately_uninhabited`
@Centril Centril mentioned this pull request Jul 24, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
@Centril
Rollup merge of rust-lang#62960 - Centril:rollup-8jay7g7, r=Centril
ab09144 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#60938 (rustdoc: make #[doc(include)] relative to the containing file)
 - rust-lang#61890 (Fix some sanity checks)
 - rust-lang#62084 (allow clippy::unreadable_literal in unicode tables)
 - rust-lang#62261 (Take substs into account in `conservative_is_privately_uninhabited`)
 - rust-lang#62528 (Add joining slices of slices with a slice separator, not just a single item)
 - rust-lang#62735 (Turn `#[global_allocator]` into a regular attribute macro)
 - rust-lang#62801 (Remove support for -Zlower-128bit-ops)
 - rust-lang#62808 (Revert "Disable stack probing for gnux32.")
 - rust-lang#62822 (Improve some pointer-related documentation)
 - rust-lang#62904 (Disable d32 on armv6 hf targets)
 - rust-lang#62907 (Initialize the MSP430 AsmParser)
 - rust-lang#62921 (Add method disambiguation help for trait implementation)
 - rust-lang#62942 (Use match ergonomics in Condvar documentation)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 25, 2019
@Centril
Rollup merge of rust-lang#62261 - varkor:conservative_is_privately_un…
44bbc38 
…inhabited-subst, r=oli-obk

Take substs into account in `conservative_is_privately_uninhabited`
@Centril Centril mentioned this pull request Jul 25, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

@bors r-

Caused a legit timeout in many a rollup... :) 😭

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 25, 2019
@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

@bors rollup=never

@JohnCSimon
Copy link
Member

@Centril @varkor Hello from triage. Any progress on this? Thanks.

@totsteps
Copy link
Member

totsteps commented Aug 13, 2019 •
edited
Loading

Second ping from triage, @varkor any updates on this? Some checks weren't successful!!

Note: Thanks for the PR. This will be closed and marked as S-inactive-closed next week. Feel free to re-open when you have time.

Thanks

@varkor
Copy link
Member Author

varkor commented Aug 13, 2019

I don't have time to investigate this now, and it's low priority, so I'll close for now.

@varkor varkor closed this Aug 13, 2019
@crlf0710
Copy link
Member

@rustbot modify labels to -S-inactive +S-inactive-closed

@rustbot rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-inactive labels Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@varkor @rust-highfive @Centril @oli-obk @bors @JohnCSimon @totsteps @crlf0710 @Mark-Simulacrum @rustbot