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

Resolve a bunch of fixmes #39384

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Resolve a bunch of fixmes #39384

merged 1 commit into from
Feb 2, 2017

Conversation

wesleywiser
Copy link
Member

Resolves 56 fixmes in test code related to box syntax.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 29, 2017

If I remember correctly, "when/if possible" == "box is stabilized" in these FIXMEs.
cc #23002 @pnkfelix

@alexcrichton
Copy link
Member

Thanks for the PR @wesleywiser I wonder though if we could perhaps avoid adding #![feature(box_syntax)] where not already specified and perhaps just delete the FIXME as the bug has already been fixed?

@wesleywiser
Copy link
Member Author

@petrochenkov My understanding from reading #22405 was that the issue was not being able to infer the result of box {expr} in various cases. This no longer seems to be the case since the tests compile.

@alexcrichton Sure I can do that. What do you think of @petrochenkov's comment? Does this need to wait for box stabilization?

@alexcrichton
Copy link
Member

Oh well it seems that the motivation/purpose of this PR is to remove these FIXME annotations. I don't think the FIXME annotations need to literally be changed to use box as it's not stable, and the relevant bug appears to have been fixed. In that sense the FIXME annotations themselves aren't too useful any more so we can just delete them without changing over to using box.

I personally prefer to favor stable features wherever possible, so that's just my own personal preference for deleting outright instead of deleting and switching to box

@wesleywiser
Copy link
Member Author

Easy enough! I'll push a new commit that simply removes the FIXMEs for this particular issue with no other changes.

@bors
Copy link
Contributor

bors commented Jan 31, 2017

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

@wesleywiser wesleywiser force-pushed the fix_fixmes branch 2 times, most recently from dc85c01 to 9265d76 Compare January 31, 2017 02:42
@alexcrichton
Copy link
Member

@wesleywiser looks like there's a pretty test failure on Travis?

@wesleywiser
Copy link
Member Author

wesleywiser commented Jan 31, 2017 via email

@wesleywiser
Copy link
Member Author

@alexcrichton It should be fixed now. It looks like two of the Mac Travis bots failed trying to clone the repo and the other two have been stuck trying to clone for a few hours now. All of the other bots have passed.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Feb 1, 2017

Are we burying the box syntax?! 😨

@ghost
Copy link

ghost commented Feb 1, 2017

There is a lot of places in the compiler where unstable features are being used, removing only the FIXME without using that lang item feels like a step backward, since this won't help pushing that feature out of the unstable zone.

@alexcrichton
Copy link
Member

@bors: r+

Thanks @wesleywiser!

@Michael-Zapata these comments are all ancient and the bugs have all been fixed anyway, so I don't think we're going backwards here at all.

@bors
Copy link
Contributor

bors commented Feb 1, 2017

📌 Commit 94687aa has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 1, 2017

⌛ Testing commit 94687aa with merge 7234e5c...

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 2, 2017

⌛ Testing commit 94687aa with merge 3b24c70...

bors added a commit that referenced this pull request Feb 2, 2017
Resolve a bunch of fixmes

Resolves 56 fixmes in test code related to `box` syntax.
@bors
Copy link
Contributor

bors commented Feb 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 3b24c70 to master...

@bors bors merged commit 94687aa into rust-lang:master Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants