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

do not promote comparing function pointers #54702

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
9 participants
@RalfJung
Member

RalfJung commented Sep 30, 2018

This could break existing code that relied on fn ptr comparison getting promoted to 'static lifetime.

Fixes #54696

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 30, 2018

Collaborator

r? @davidtwco

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

Collaborator

rust-highfive commented Sep 30, 2018

r? @davidtwco

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

&(main as fn() == main as fn());
// Also check nested case
&(&(main as fn()) == &(main as fn()));
}

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Sep 30, 2018

Member

How does this test verify lack of promotion? Shouldn't we instead have a compile-fail test?

@Mark-Simulacrum

Mark-Simulacrum Sep 30, 2018

Member

How does this test verify lack of promotion? Shouldn't we instead have a compile-fail test?

This comment has been minimized.

@arielb1

arielb1 Sep 30, 2018

Contributor

The promotion causes crashes, because function pointer equality can't be done in constants

@arielb1

arielb1 Sep 30, 2018

Contributor

The promotion causes crashes, because function pointer equality can't be done in constants

This comment has been minimized.

@RalfJung

RalfJung Sep 30, 2018

Member

I can also add a compile-fail test if you want -- though there is no fundamental reason we shouldn't want to promote this in the future, so that test wouldn't be "normative". OTOH, making this code run (and not emit a trap) is "normative".

@RalfJung

RalfJung Sep 30, 2018

Member

I can also add a compile-fail test if you want -- though there is no fundamental reason we shouldn't want to promote this in the future, so that test wouldn't be "normative". OTOH, making this code run (and not emit a trap) is "normative".

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Sep 30, 2018

Contributor

This could break existing code that relied on fn ptr comparison getting promoted to 'static lifetime.

That code is broken anyway

Contributor

arielb1 commented Sep 30, 2018

This could break existing code that relied on fn ptr comparison getting promoted to 'static lifetime.

That code is broken anyway

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco
Member

davidtwco commented Sep 30, 2018

@rust-highfive rust-highfive assigned arielb1 and unassigned davidtwco Sep 30, 2018

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Sep 30, 2018

Member

That code is broken anyway

Fair enough.

Member

RalfJung commented Sep 30, 2018

That code is broken anyway

Fair enough.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Sep 30, 2018

Contributor

r? @eddyb

Contributor

arielb1 commented Sep 30, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned arielb1 Sep 30, 2018

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Oct 1, 2018

Contributor

@bors r+

Contributor

oli-obk commented Oct 1, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2018

Contributor

📌 Commit 4cbfc93 has been approved by oli-obk

Contributor

bors commented Oct 1, 2018

📌 Commit 4cbfc93 has been approved by oli-obk

@oli-obk oli-obk assigned oli-obk and unassigned eddyb Oct 1, 2018

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Oct 1, 2018

Member

@RalfJung does this also fix #54685 or is that a different issue? That one is also comparing function pointers but the discussion is focused on the LLVM side.

Member

dtolnay commented Oct 1, 2018

@RalfJung does this also fix #54685 or is that a different issue? That one is also comparing function pointers but the discussion is focused on the LLVM side.

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 1, 2018

Member

@dtolnay That's a different issue, but thanks for pointing me to it.

Member

RalfJung commented Oct 1, 2018

@dtolnay That's a different issue, but thanks for pointing me to it.

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 2, 2018

Rollup merge of rust-lang#54702 - RalfJung:fn-ptr-promotion, r=oli-obk
do not promote comparing function pointers

This *could* break existing code that relied on fn ptr comparison getting promoted to `'static` lifetime.

Fixes rust-lang#54696

bors added a commit that referenced this pull request Oct 2, 2018

Auto merge of #54763 - pietroalbini:rollup, r=pietroalbini
Rollup of 10 pull requests

Successful merges:

 - #54269 (#53840: Consolidate pattern check errors)
 - #54391 (suggest `crate::...` for "local" paths in 2018)
 - #54603 (Add `crate::` to trait suggestions in Rust 2018.)
 - #54648 (Update Cargo's submodule)
 - #54680 (make run-pass tests with empty main just compile-pass tests)
 - #54687 (Use impl_header_lifetime_elision in libcore)
 - #54699 (Re-export `getopts` so custom drivers can reference it.)
 - #54702 (do not promote comparing function pointers)
 - #54728 (Renumber `proc_macro` tracking issues)
 - #54745 (make `CStr::from_bytes_with_nul_unchecked()` a const fn)

Failed merges:

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 2, 2018

Rollup merge of rust-lang#54702 - RalfJung:fn-ptr-promotion, r=oli-obk
do not promote comparing function pointers

This *could* break existing code that relied on fn ptr comparison getting promoted to `'static` lifetime.

Fixes rust-lang#54696

bors added a commit that referenced this pull request Oct 2, 2018

Auto merge of #54767 - pietroalbini:rollup, r=pietroalbini
Rollup of 10 pull requests

Successful merges:

 - #54269 (#53840: Consolidate pattern check errors)
 - #54458 (Allow both explicit and elided lifetimes in the same impl header)
 - #54603 (Add `crate::` to trait suggestions in Rust 2018.)
 - #54648 (Update Cargo's submodule)
 - #54680 (make run-pass tests with empty main just compile-pass tests)
 - #54687 (Use impl_header_lifetime_elision in libcore)
 - #54699 (Re-export `getopts` so custom drivers can reference it.)
 - #54702 (do not promote comparing function pointers)
 - #54728 (Renumber `proc_macro` tracking issues)
 - #54745 (make `CStr::from_bytes_with_nul_unchecked()` a const fn)

Failed merges:

r? @ghost

@bors bors merged commit 4cbfc93 into rust-lang:master Oct 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment