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

Intrinsic overhaul #7254

Closed
wants to merge 10 commits into from
Closed

Intrinsic overhaul #7254

wants to merge 10 commits into from

Conversation

Blei
Copy link
Contributor

@Blei Blei commented Jun 20, 2013

This sets the get_tydesc() return type correctly and removes the intrinsic module. See #3730, #3475.

Update: this now also removes the unused shape fields in tydescs.

@emberian
Copy link
Member

Outdated!
benchmark

@brson
Copy link
Contributor

brson commented Jun 20, 2013

Awesome. I've been wanting this forever. Instead of moving the visitor lang items to unstable::intrinsics can we move them somewhere else, maybe unstable::visitor (or maybe they belong in one of the reflection modules. @graydon may have an opinion)? I'd prefer to keep the intrinsics module simply the collection of rust-intrinsic ABI functions.

@brson
Copy link
Contributor

brson commented Jun 20, 2013

Hm, I guess there's a close relationship between these lang items and the intrinsics since they show up in the intrinsic signatures. Maybe someone else has an opinion.

@Blei
Copy link
Contributor Author

Blei commented Jun 21, 2013

Update: this now also removes rust_call_tydesc_glue from rt.

@Blei
Copy link
Contributor Author

Blei commented Jun 22, 2013

Rebased on top of the trans refactoring.

@Blei
Copy link
Contributor Author

Blei commented Jun 22, 2013

Update: added a commit that removes the unused tydesc argument of glue functions.

@emberian
Copy link
Member

Memory usage (fairly accurate timewise this time)

mem benc

Without memory profiling, it's a few (~3) seconds faster.

@graydon
Copy link
Contributor

graydon commented Jun 23, 2013

Great. I don't especially care where they get relocated to. Intrinsic wasn't an especially accurate name for this sort of thing. Maybe just lang items in reflect.

Blei added 9 commits June 23, 2013 12:46
This fixes part of #3730, but not all.
Also changes the TyDesc struct to be equivalent with the generated
code, with the hope that the above issue may one day be closed for good,
i.e. that the TyDesc type can completely be specified in the Rust
sources and not be generated.
To achieve this, the following changes were made:
* Move TyDesc, TyVisitor and Opaque to std::unstable::intrinsics
* Convert TyDesc, TyVisitor and Opaque to lang items instead of specially
  handling the intrinsics module
* Removed TypeDesc, FreeGlue and get_type_desc() from sys

Fixes #3475.
Towards #4812. Also includes some minor cleanups.
TyDesc, TyVisitor and intrinsic are not used anymore.
To remove the environment pointer, support for function pointers without
an environment argument is needed (i.e. a fixed version of #6661).
@Blei
Copy link
Contributor Author

Blei commented Jun 24, 2013

I tried to debug the failure on windows, but didn't get very far. gdb backtraces told me that it appears to fail an assert in LLVMSetFunctionCallConv, which is called from decl_fn in trans/base.rs. A little more investigation led to the conclusion, that the value returned by LLVMGetOrInsertFunction (also called in decl_fn) is not a valid LLVM function. This happens when inserting the function with name free. But I have no idea why this function, and why this would be related to my patches.

@Blei
Copy link
Contributor Author

Blei commented Jun 24, 2013

I bisected the failure, the offending commit is 469f394, the second commit.

@Blei
Copy link
Contributor Author

Blei commented Jun 24, 2013

The failure on windows should be fixed now.

This patch ensures that the multiple extern definitions of `free` in the
run-pass tests have the same declaration, working around #7352.
bors added a commit that referenced this pull request Jun 24, 2013
This sets the `get_tydesc()` return type correctly and removes the intrinsic module. See #3730, #3475.

Update: this now also removes the unused shape fields in tydescs.
@Blei
Copy link
Contributor Author

Blei commented Jun 25, 2013

The latest merge attempt seems to have been interrupted manually while building LLVM. Could I get a retry?

bors added a commit that referenced this pull request Jun 25, 2013
This sets the `get_tydesc()` return type correctly and removes the intrinsic module. See #3730, #3475.

Update: this now also removes the unused shape fields in tydescs.
@bors bors closed this Jun 25, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 3, 2021
Move `needless_borrow` to style

fixes: rust-lang#3742

rust-lang#7105 should be merged first to fix the false positive.

changelog: move `needless_borrow` from `nursery` to `style`
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.

5 participants