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

address some FIXME whose associated issues were marked as closed #44943

Merged
merged 4 commits into from
Oct 5, 2017
Merged

address some FIXME whose associated issues were marked as closed #44943

merged 4 commits into from
Oct 5, 2017

Conversation

nivkner
Copy link
Contributor

@nivkner nivkner commented Sep 30, 2017 •
edited
Loading

part of #44366

@nivkner
address some FIXMEs whose associated issues were marked as closed
ff99111 
remove FIXME(rust-lang#13101) since `assert_receiver_is_total_eq` stays.
remove FIXME(rust-lang#19649) now that stability markers render.
remove FIXME(rust-lang#13642) now the benchmarks were moved.
remove FIXME(rust-lang#6220) now that floating points can be formatted.
remove FIXME(rust-lang#18248) and write tests for `Rc<str>` and `Rc<[u8]>`
remove reference to irelevent issues in FIXME(rust-lang#1697, rust-lang#2178...)
update FIXME(rust-lang#5516) to point to getopts issue 7
update FIXME(rust-lang#7771) to point to RFC 628
update FIXME(rust-lang#19839) to point to issue 26925
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nivkner nivkner changed the title address some FIXMEs whose associated issues were marked as closed address some FIXME whose associated issues were marked as closed Sep 30, 2017
petrochenkov
petrochenkov reviewed Sep 30, 2017
View reviewed changes
src/librustc/hir/mod.rs Outdated
@@ -916,8 +916,7 @@ impl Stmt_ {
}
}

// FIXME (pending discussion of #1697, #2178...): local should really be
// a refinement on pat.
// FIXME: local should really be a refinement on pat.
Copy link
Contributor

Choose a reason for hiding this comment

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

What this FIXME even mean? I'd rather remove it entirely (and its equivalent in ast.rs as well).
#1697 #2178 looks irrelevant.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2017
@carols10cents
Copy link
Member

Thanks for the PR! We’ll periodically check in on it to make sure that @dtolnay or someone else from the team reviews it soon.

dtolnay
dtolnay reviewed Oct 3, 2017
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks so much for tackling this! Looks good to me except for the one that I reopened.

src/libcore/benches/mem.rs
@@ -10,7 +10,6 @@

use test::Bencher;

// FIXME #13642 (these benchmarks should be in another place)
Copy link
Member

Choose a reason for hiding this comment

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

I reopened this issue because I think it was closed prematurely. Please keep this FIXME for now (or fix #13642!).

@nivkner
Copy link
Contributor Author

nivkner commented Oct 4, 2017 •
edited
Loading

Separated the benchmarks in libcore/benches/mem.rs since they weren't really related to each other.

Moved to librustc since the benchmarks are about language features not items in libcore, and that seems like the place where said features are defined.

That should hopefully fix #13642 for real this time.

dtolnay
dtolnay reviewed Oct 4, 2017
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for sorting out those benchmarks! I think you need to pick a different module name though.

src/librustc/benches/lib.rs Outdated
extern crate test;

mod dispatch;
mod match;
Copy link
Member

Choose a reason for hiding this comment

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

error: expected identifier, found keyword `match`
  --> benches/lib.rs:19:5
   |
19 | mod match;
   |     ^^^^^

@dtolnay
Copy link
Member

dtolnay commented Oct 4, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2017

📌 Commit 559adb7 has been approved by dtolnay

dtolnay
dtolnay reviewed Oct 5, 2017
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I checked off these changes in the checklist in #44366. We are just waiting for this to make it to the top of the build queue and pass tests on all platforms.

@bors
Copy link
Contributor

bors commented Oct 5, 2017

⌛ Testing commit 559adb7 with merge 417c738...

bors added a commit that referenced this pull request Oct 5, 2017
@bors
Auto merge of #44943 - nivkner:fixme_fixup, r=dtolnay
417c738 
address some FIXME whose associated issues were marked as closed

part of #44366
@bors
Copy link
Contributor

bors commented Oct 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing 417c738 to master...

@bors bors merged commit 559adb7 into rust-lang:master Oct 5, 2017
@nivkner nivkner deleted the fixme_fixup branch October 7, 2017 09:16
@cuviper cuviper mentioned this pull request Oct 17, 2017
Merged
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Dec 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nivkner @rust-highfive @carols10cents @dtolnay @bors @petrochenkov