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

Fix various bugs around empty structs and patterns #29383

Merged
merged 4 commits into from Nov 28, 2015

Conversation

Projects
None yet
7 participants
@petrochenkov
Contributor

petrochenkov commented Oct 26, 2015

Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens UnitVariant(..) for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 26, 2015

r? @pnkfelix

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 26, 2015

Heh, yeah, I like to do Foo(..) if I only care about the variant, so that it is forwards compatible with adding parameters later (which happens pretty regularly).

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 27, 2015

This PR looks fine to me. It does need a crater run.

(I have not yet successfully run crater but I'm wiling to give it another shot.)

@bors

This comment has been minimized.

Contributor

bors commented Nov 2, 2015

☔️ The latest upstream changes (presumably #28846) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov force-pushed the petrochenkov:empstr branch from 040ad0f to 40717ca Nov 2, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 2, 2015

Rebased.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 2, 2015

@petrochenkov great; I am seeing if I can do a crater run now with it.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 2, 2015

I've made a new needs-crater tag on GitHub so we can keep track of which PRs need a crater run so we know how to move them forward

@bors

This comment has been minimized.

Contributor

bors commented Nov 4, 2015

☔️ The latest upstream changes (presumably #29217) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov force-pushed the petrochenkov:empstr branch from 40717ca to b7f8311 Nov 4, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 4, 2015

Rebased.

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2015

☔️ The latest upstream changes (presumably #29581) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 5, 2015

@petrochenkov thanks for rebasing

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 5, 2015

I did a crater run (on the revisions from before the rebase).

Zero regressions.

crater results here: https://gist.github.com/pnkfelix/4b800d1faf17eb494d8e


Update: I just looked at those results again, and realized that, if I understand them correctly, they are saying that all crates failed to build on both of my compilers. :(

I'm not quite sure what I did wrong, if anything. I'll try a new run on the rebased versions.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 5, 2015

(So, r=me after next rebase.)


Update: see above comment. The aforementioned crater run seemed problematic, not b/c of this PR, but probably user error.

@petrochenkov petrochenkov force-pushed the petrochenkov:empstr branch from b7f8311 to fec3902 Nov 5, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 5, 2015

Rebased.

@bors

This comment has been minimized.

Contributor

bors commented Nov 11, 2015

☔️ The latest upstream changes (presumably #29763) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov force-pushed the petrochenkov:empstr branch from fec3902 to 9942e4e Nov 12, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 12, 2015

Rebased.

@alexcrichton, could you run crater on this?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 15, 2015

Starting a crater run.

@bors

This comment has been minimized.

Contributor

bors commented Nov 16, 2015

☔️ The latest upstream changes (presumably #29387) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 16, 2015

Crater reports eight root regressions with as many as up to 34. I glanced at the root ones and they at least all look legitimate.

Perhaps that error could be a warning for a few cycles? Seems like a good bugfix to have, but may be good to give fair warning.

@petrochenkov petrochenkov force-pushed the petrochenkov:empstr branch from afe9350 to af96402 Nov 19, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 19, 2015

Rebased.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 25, 2015

ping @pnkfelix
beta is soon

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 27, 2015

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 27, 2015

📌 Commit af96402 has been approved by pnkfelix

@bors

This comment has been minimized.

Contributor

bors commented Nov 28, 2015

⌛️ Testing commit af96402 with merge f0fcef8...

bors added a commit that referenced this pull request Nov 28, 2015

Auto merge of #29383 - petrochenkov:empstr, r=pnkfelix
Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.
@bors

This comment has been minimized.

Contributor

bors commented Nov 28, 2015

💔 Test failed - auto-mac-32-opt

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2015

fatal: unable to connect to github.com:
github.com[0: 192.30.252.130]: errno=Operation timed out

Needs retry

@bluss

This comment has been minimized.

Contributor

bluss commented Nov 28, 2015

@bors retry

@bors

This comment has been minimized.

Contributor

bors commented Nov 28, 2015

⌛️ Testing commit af96402 with merge e9ac440...

bors added a commit that referenced this pull request Nov 28, 2015

Auto merge of #29383 - petrochenkov:empstr, r=pnkfelix
Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.

@bors bors merged commit af96402 into rust-lang:master Nov 28, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
@bluss

This comment has been minimized.

Contributor

bluss commented Nov 30, 2015

It looks like it's a breaking change anyway, conv 0.3.1 0.3.0 + git

src/errors.rs:576:17: 576:32 warning: `NegOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:576:17: 576:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:576:17: 576:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for det
ails.
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:577:17: 577:32 warning: `PosOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:577:17: 577:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:577:17: 577:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:602:17: 602:32 warning: `NegOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:602:17: 602:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:602:17: 602:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:603:17: 603:32 warning: `PosOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:603:17: 603:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:603:17: 603:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:574:9: 578:10 error: non-exhaustive patterns: `Err(_)` not covered [E0004]
src/errors.rs:574         match self.map_err(Into::into) {
src/errors.rs:575             Ok(v) => v,
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
src/errors.rs:578         }
src/errors.rs:574:9: 578:10 help: run `rustc --explain E0004` to see a detailed explanation
src/errors.rs:600:9: 604:10 error: non-exhaustive patterns: `Err(_)` not covered [E0004]
src/errors.rs:600         match self.map_err(Into::into) {
src/errors.rs:601             Ok(v) => v,
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
src/errors.rs:604         }

m4rw3r added a commit to m4rw3r/rust-conv that referenced this pull request Nov 30, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 30, 2015

Hm, the warning should not affect exhaustiveness check, I'll investigate what happens.

@bluss

This comment has been minimized.

Contributor

bluss commented Nov 30, 2015

I apologise if I'm pointing to the wrong PR, but it seems related.

bors added a commit that referenced this pull request Jan 11, 2016

Auto merge of #30753 - pnkfelix:downgrade-29383-struct-warnings-to-er…
…rors, r=nikomatsakis

Downgrade unit struct match via S(..) warnings to errors

The error signalling was introduced in #29383

It was noted as a warning-cycle-less regression in #30379

Fix #30379

bors added a commit that referenced this pull request Jan 11, 2016

Auto merge of #30753 - pnkfelix:downgrade-29383-struct-warnings-to-er…
…rors, r=nikomatsakis

Downgrade unit struct match via S(..) warnings to errors

The error signalling was introduced in #29383

It was noted as a warning-cycle-less regression in #30379

Fix #30379

@petrochenkov petrochenkov deleted the petrochenkov:empstr branch Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment