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

More robust type_eq_non_static implementation #14

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Sep 19, 2023

According to #6 (comment) in reference to the type_id_of::<T> as usize-based implementation:

As for the function instantiation address taking thing: it simply looks broken/unsound - the only reason LLVM mergefunc would even consider them distinct is its implementation being too simple and nowhere near "graph isomorphism" (remember that we have unnamed_addr semantics for functions, which makes it valid to merge functions even if their addresses are observed).

I am being told by lcnr (also working on polymorphization) that this will break at some point the future (assuming we get to turn that on by default), so this is not that theoretical.

@dtolnay
Copy link
Contributor Author

dtolnay commented Sep 19, 2023

1.38.0 macos failure in CI is rust-lang/rust#105167, unrelated to this PR.

@sagebind
Copy link
Owner

Alright, CI should pass now if you pull in the latest master commits into your branch.

Copy link
Owner

@sagebind sagebind left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. The prior implementation always did make me uncomfortable.

@sagebind sagebind merged commit 42f0024 into sagebind:master Sep 19, 2023
7 checks passed
@sagebind
Copy link
Owner

Thanks for working on this!

@dtolnay dtolnay deleted the nonstatictypeid branch September 19, 2023 20:00
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.

2 participants