Skip to content

Conversation

jackh726
Copy link
Member

@jackh726 jackh726 commented Jun 3, 2020

Things done in this PR:

  • Upgrade Chalk to 0.11.0
  • Added compare-mode=chalk
  • Bump rustc-hash in librustc_data_structures to 1.1.0 to match Chalk
  • Removed RustDefId since the builtin type support is there
  • Add a few more FIXME(chalk)s for problem spots I hit when running all tests with chalk
  • Added some more implementation code for some newer builtin Chalk types (e.g. FnDef, Array)
  • Lower RegionOutlives and ObjectSafe predicates
  • Lower Dyn without the region
  • Handle Int/Float CanonicalVarKinds
  • Uncomment some Chalk tests that actually work now
  • Remove the revisions in src/test/ui/coherence/coherence-subtyping.rs since they aren't doing anything different

r? @nikomatsakis

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

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Looking good! Exciting to see the progress here. Left some nits.

@bors
Copy link
Collaborator

bors commented Jun 3, 2020

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

@jackh726
Copy link
Member Author

jackh726 commented Jun 3, 2020

@nikomatsakis addressed your comments

  • Removed the commented out assert
  • Changed skip_binders to no_bound_vars().expect(). This requires collecting into an intermediate Vec, since inputs() returns a slice, which doesn't impl TypeFoldable. But since this is going to be fixed on the Chalk side anyways, I don't think this is a problem right now. It did catch the fact that we were ignoring this in the inherent_impl test, so I commented out the bits of code that fail now :(
  • Can't change InternedAdtDef to AdtDef since it has a Copy bound and AdtDef isn't.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 4, 2020

📌 Commit fc8e10d5965c1d4954706cc080ba3a1b0bf06879 has been approved by nikomatsakis

@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 Jun 4, 2020
@bors
Copy link
Collaborator

bors commented Jun 6, 2020

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

@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 Jun 6, 2020
@jackh726
Copy link
Member Author

jackh726 commented Jun 7, 2020

I rebased. Can I get this r+ed again? @nikomatsakis

@Aaron1011
Copy link
Contributor

@jackh726: It looks like you've modified the llvm-project submodule: was that intentional?

@jackh726 jackh726 force-pushed the chalk-more branch 3 times, most recently from 6f44abe to 4cf2833 Compare June 8, 2020 03:25
@jackh726
Copy link
Member Author

jackh726 commented Jun 8, 2020

@Aaron1011 😔 no that happened during rebase. Fixed.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 8, 2020

📌 Commit 4cf2833 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2020
@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Collaborator

bors commented Jun 12, 2020

⌛ Testing commit 4cf2833 with merge d73e24b4f387054673f9ca9fa3a9deb3d6b73ba4...

@bors
Copy link
Collaborator

bors commented Jun 12, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 12, 2020
@Aaron1011
Copy link
Contributor

My minimized example from #73335 no longer causes Rustdoc to OOM after #73452 was merged. This PR should now be unblocked.

@Aaron1011 Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 20, 2020
@jackh726
Copy link
Member Author

@nikomatsakis care to r+ again?

@Dylan-DPC-zz
Copy link

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Jun 20, 2020

📌 Commit d63195b has been approved by nikomatsakis

@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 Jun 20, 2020
@bors
Copy link
Collaborator

bors commented Jun 21, 2020

⌛ Testing commit d63195b with merge a8cf399...

@bors
Copy link
Collaborator

bors commented Jun 21, 2020

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing a8cf399 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2020
@bors bors merged commit a8cf399 into rust-lang:master Jun 21, 2020
@jackh726 jackh726 deleted the chalk-more branch June 21, 2020 22:28
@nnethercote
Copy link
Contributor

This was a moderate perf win. Nice job!

@jackh726
Copy link
Member Author

@nnethercote that's really unexpected, since basically all of this code doesn't run unless you're in -Z chalk. The only thing I can imagine giving a perf win would be the rustc_hash bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

10 participants