Skip to content

/rust

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

[MIR] Implement translation of references to various items #29907

Merged
merged 4 commits into from Dec 18, 2015

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@nagisa @bors @nikomatsakis
@nagisa
Copy link
Contributor

nagisa commented Nov 18, 2015

Still will not translate references to items like X or Y::V where

struct X;
enum Y { V }

but I must go work on university things so I’m PRing what I have.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Nov 18, 2015

@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nagisa commented Nov 18, 2015

Nits addressed.

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from 53aa6b3 to 6bb92a5 Nov 18, 2015

src/librustc_trans/trans/mir/did.rs Outdated
substs: &'tcx Substs<'tcx>,
did: DefId)
-> OperandRef<'tcx> {
match ty.sty {

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Nov 18, 2015

Contributor

Checking the type of the item like this feels like a horrible hack. Perhaps we should just be including more information in the Literal MIR node? Or a Def (as I think you suggested on IRC)? Also, I haven't gone digging into trans, but is this code here a kind if port of code that exists elsewhere?

This comment has been minimized.

Sign in to view
Copy link
@nagisa

nagisa Nov 18, 2015

Author Contributor

Also, I haven't gone digging into trans, but is this code here a kind if port of code that exists elsewhere?

Nope. The trans_fn_ref below is a simplified “port”, but I haven’t found anything in old trans which would do matching like this (AFAICT it gathers such information from various other sources, esp. HIR).

Perhaps we should just be including more information in the Literal MIR node? Or a Def (as I think you suggested on IRC)?

This approach makes it pretty easy to match whether enum/struct references should be translated as functions or not. I’m not sure other methods would allow checking for this as easily.

I’m willing to explore for better methods to match correct path, but won’t be able to dedicate much time for that in coming weeks.

This comment has been minimized.

Sign in to view
Copy link
@nagisa

nagisa Nov 20, 2015

Author Contributor

Okay, I’m convinced now this is not only a hack but is also wrong (e.g. it will panic compiling a function that’s defined in impl block).

However, I won’t be able to get back to it for at least a week and a half. I’ll go ahead and mark the issue as [WIP] and hopefully work on it later.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 18, 2015

☔️ The latest upstream changes (presumably #29886) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch 2 times, most recently from 9a2607e to f013dd2 Nov 18, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 20, 2015

☔️ The latest upstream changes (presumably #29534) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from a3485cc to 250d9e7 Nov 20, 2015

@nagisa nagisa changed the title [MIR] Implement translation of references to various items [WIP][MIR] Implement translation of references to various items Nov 20, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 1, 2015

☔️ The latest upstream changes (presumably #25570) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from 250d9e7 to 3673ee5 Dec 5, 2015

@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nagisa commented Dec 5, 2015

I changed my approach to storing relevant kind inside Literal itself. Current test failures result from #30202 I think.

I’m not entirely sure I’m using the correct substs in all the correct locations, but things seem to be working; something to be reviewed more carefully.

@nagisa nagisa changed the title [WIP][MIR] Implement translation of references to various items [MIR] Implement translation of references to various items Dec 5, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 11, 2015

☔️ The latest upstream changes (presumably #30301) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from f02c51a to 09033dc Dec 11, 2015

@nagisa nagisa referenced this pull request Dec 13, 2015

Merged

[MIR] Initial implementation for translating calls. #30364

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from 09033dc to f95aae4 Dec 15, 2015

@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nagisa commented Dec 15, 2015

(I hate how github removed all the comments on force push :()

nagisa@ece05d3 and nagisa@5fb2edc were the two commits that had comments.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link

nikomatsakis commented on src/librustc_trans/trans/consts.rs in b3720ea Dec 17, 2015

can you add some sort of test for these?
@nagisa
Test generic methods
Loading status checks…
7dd9579

@nagisa nagisa force-pushed the nagisa:mir-moar-constants branch from f95aae4 to 7dd9579 Dec 17, 2015

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Dec 17, 2015

@bors r+

Still more refactoring to be done here but this seems like an epic improvement! Thanks @nagisa!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 17, 2015

📌 Commit 7dd9579 has been approved by nikomatsakis

bors added a commit that referenced this pull request Dec 18, 2015

@bors
Auto merge of #29907 - nagisa:mir-moar-constants, r=nikomatsakis 
Still will not translate references to items like `X` or `Y::V` where

```
struct X;
enum Y { V }
```

but I must go work on university things so I’m PRing what I have.

r? @nikomatsakis
89ded8e

bors added a commit that referenced this pull request Dec 18, 2015

@bors
Auto merge of #29907 - nagisa:mir-moar-constants, r=nikomatsakis 
Still will not translate references to items like `X` or `Y::V` where

```
struct X;
enum Y { V }
```

but I must go work on university things so I’m PRing what I have.

r? @nikomatsakis
4eadabd
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 18, 2015

⌛️ Testing commit 7dd9579 with merge 4eadabd...
@bors

@bors bors merged commit 7dd9579 into rust-lang:master Dec 18, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.