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

Implement the Fn trait for bare fn pointers in the compiler #19449

Merged
merged 3 commits into from
Dec 4, 2014

Conversation

nikomatsakis
Copy link
Contributor

Implement the Fn trait for bare fn pointers in the compiler rather
than doing it using hard-coded impls. This means that it works also
for more complex fn types involving bound regions.

@nikomatsakis
Copy link
Contributor Author

r? @pcwalton

@japaric
Copy link
Member

japaric commented Dec 2, 2014

Could it be that this fixes #19126? 😍 I'll give it a go!

@nikomatsakis
Copy link
Contributor Author

@japaric that was the idea :)

@reem
Copy link
Contributor

reem commented Dec 2, 2014

Very happy to see this being fixed :)

@japaric
Copy link
Member

japaric commented Dec 2, 2014

@nikomatsakis I always get this LLVM assertion:

  • when bootstrapping rust, specifically during stage0/libcollections compilation
  • when using a compiler that includes this PR as the stage0 compiler
  • on a branch that includes this PR and unboxes the closures in the Option methods
  • but only if I use the -C codegen_units flag:
rustc: /root/rust/src/llvm/lib/IR/Constants.cpp:1609: static llvm::Constant* llvm::ConstantExpr::getPointerCast(llvm::Constant*, llvm::Type*): Assertion `S->getType()->isPtrOrPtrVectorTy() && "Invalid cast"' failed.

I'll investigate it later, and open an issue with some minimal reproduction script. (But my guess, is that is something cross-crate related)

@nikomatsakis
Copy link
Contributor Author

@japaric hmm, ok. I'll add some cross-crate tests. It also occurs to me I forgot to add the cache that I meant to add, so right now it generates fresh adapters as needed.

@nikomatsakis
Copy link
Contributor Author

The requirement for the -C codegen-units flag is interesting though...

};

let arguments_tuple = ty::mk_tup(self.tcx(), sig.inputs.to_vec());
let output_type = sig.output.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you cloned the sig above, why not just move inputs and output out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, holdover from an older version where I needed the clone for something

…than doing it using hard-coded impls. This means that it works also for more complex fn types involving bound regions. Fixes rust-lang#19126.
bors added a commit that referenced this pull request Dec 4, 2014
…cwalton

Implement the `Fn` trait for bare fn pointers in the compiler rather
than doing it using hard-coded impls. This means that it works also
for more complex fn types involving bound regions.
@bors bors closed this Dec 4, 2014
@bors bors merged commit f2731ff into rust-lang:master Dec 4, 2014
@japaric
Copy link
Member

japaric commented Dec 4, 2014

Could someone make a snapshot containing this change? This is needed to land the migration to unboxed closures patch.

cc @aturon @alexcrichton

@alexcrichton
Copy link
Member

One should be on the way!

@nikomatsakis nikomatsakis deleted the unboxed-closure-fn-impl branch March 30, 2016 16:17
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.

None yet

6 participants