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

Remove unused implicit argument for functions that return immediate values #6731

Merged
merged 4 commits into from
May 29, 2013

Conversation

thomaslee
Copy link
Contributor

Fix for #6575. In the trans phase, rustc emits code for a function parameter that goes completely unused in the event the return type of the function in question happens to be an immediate.

This patch modifies rustc & parts of rustrt to ensure that the vestigial parameter is no longer present in compiled code.

@thomaslee
Copy link
Contributor Author

@catamorphism alright, good news is with the changes to the build system all tests are passing & everything looks swell but I need some eyes on one particular LOC:

thomaslee@c298c51#L4R158

It seems like this code is relying on the old behaviour but given it's not breaking anything obvious it's hard for me to know whether this is something I need to be concerned about.

Once I get some clarification on that, I'll see if I can put together a test for this.

Also, seems like this stuff is going to conflict pretty badly with @nikomatsakis' work on #6661.

@nikomatsakis
Copy link
Contributor

At least from a quick look over the patch, I don't think the conflicts should be too bad. I'm hoping #6661 will land tonight. If it does, ping me on IRC (nmatsakis) and I'll help you through resolving the conflicts if you have trouble. I've tried to cleanup and comment the logic for function calls and in particular foreign function calls, so hopefully it won't be too hard to figure out.

@nikomatsakis
Copy link
Contributor

Hi---thanks for the patch! I added some comments. It looks pretty good, I'd just like to see one minor stylistic fix (not returning the immediate flag but rather using methods on the FnCtxt). Obviously, it needs rebase, and yes it will conflict with #6661 but we'll deal with it. #6661 still has not landed (mired in niggly ABI details of various platforms, but getting closer) so whichever lands first the other can rebase.

@pnkfelix
Copy link
Member

(I think Niko has been typing #6111 when he means to be typing #6661, but since I'm not 100% sure of that, I'll let Niko fix the references in his comments.)

@nikomatsakis
Copy link
Contributor

Indeed @pnkfelix was correct.

@thomaslee
Copy link
Contributor Author

Thanks for taking a look at this @nikomatsakis, I've implemented the changes you suggested.

LLVM newbie question: will this leak? thomaslee@b7f71e1#L0L1618

(Note I reassign fcx.llenv further down)

@nikomatsakis
Copy link
Contributor

LLVM newbie question: will this leak? thomaslee@b7f71e1#L0L1618

No.

bors added a commit that referenced this pull request May 29, 2013
Fix for #6575. In the trans phase, rustc emits code for a function parameter that goes completely unused in the event the return type of the function in question happens to be an immediate.

This patch modifies rustc & parts of rustrt to ensure that the vestigial parameter is no longer present in compiled code.
@bors bors closed this May 29, 2013
@bors bors merged commit b7f71e1 into rust-lang:incoming May 29, 2013
@thomaslee thomaslee deleted the issue-6575 branch May 29, 2013 05: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.

5 participants