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

expose needs_drop under mem #41892

Merged
merged 3 commits into from
May 21, 2017
Merged

expose needs_drop under mem #41892

merged 3 commits into from
May 21, 2017

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented May 10, 2017

Note that I purposefully didn't update the intrinsic's documentation, because I think it makes sense for it be more... "honest" about its semantics.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@TimNN TimNN added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2017
@nagisa nagisa mentioned this pull request May 11, 2017
Closed
@Gankra
Copy link
Contributor Author

Gankra commented May 11, 2017

Fixed the whitespace errors (had auto-whitespace-trimming disabled since Swift people got mad when you fixed that).

The unstable book error thing is uh, new to me.

@aturon
Copy link
Member

aturon commented May 12, 2017

cc @rust-lang/libs @rust-lang/lang

@alexcrichton
Copy link
Member

👍

eddyb
eddyb reviewed May 12, 2017
View reviewed changes
src/libcore/mem.rs
///
/// This is purely an optimization hint, and may be implemented conservatively.
/// For instance, always returning `true` would be a valid implementation of
/// this function.
Copy link
Member

Choose a reason for hiding this comment

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

So there's some backstory here: basically this was never implemented in the compiler in a principled way, to account for associated types. I've made the Copy check recursive recently but I don't believe that's relevant here.
What's left is to normalize associated type projections in the recursion and/or make an auto trait and rely on the trait system to handle the structural recursion involved here. cc @nikomatsakis

Copy link
Contributor

@gnzlbg gnzlbg Sep 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@eddyb Is this a bug in the current implementation, and if so, is there an issue tracking it? If not, could you maybe expand on why needs_drop cannot be accurate?

@bors
Copy link
Contributor

bors commented May 13, 2017

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

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2017
@alexcrichton
Copy link
Member

ping @gankro looks like this needs a rebase? Looks like there's not a whole lot of opinions here, and we don't have a huge amount of ceremony about unstable apis, so I can r+ when you've rebased

@Mark-Simulacrum Mark-Simulacrum added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 20, 2017
@Mark-Simulacrum
Copy link
Member

@aturon You nominated but didn't label with any teams, I've added both that you pinged (T-lang and T-libs).

@Gankra
Copy link
Contributor Author

Gankra commented May 20, 2017

Rebased.

@Mark-Simulacrum
Copy link
Member

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented May 20, 2017

📌 Commit 1f01b09 has been approved by alexcrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 20, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#41892 - Gankro:needs_drop, r=alexcrichton
c37d66a 
expose needs_drop under mem

Note that I purposefully didn't update the intrinsic's documentation, because I think it makes sense for it be more... "honest" about its semantics.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 20, 2017
Merged
bors added a commit that referenced this pull request May 21, 2017
@bors
Auto merge of #42130 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
7ac844f 
Rollup of 6 pull requests

- Successful merges: #41892, #42062, #42091, #42093, #42098, #42127
- Failed merges:
@bors
Copy link
Contributor

bors commented May 21, 2017

⌛ Testing commit 1f01b09 with merge 7ac844f...

@bors bors merged commit 1f01b09 into rust-lang:master May 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

@gnzlbg gnzlbg gnzlbg left review comments

Assignees

@brson brson

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Gankra @rust-highfive @aturon @alexcrichton @bors @Mark-Simulacrum @eddyb @gnzlbg @brson @TimNN