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.

Sign up for GitHub

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

Jump to bottom

Collector in trans is private #44334

Closed
MaikKlein opened this issue Sep 5, 2017 · 5 comments
Closed

Collector in trans is private #44334

MaikKlein opened this issue Sep 5, 2017 · 5 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@MaikKlein
Copy link
Contributor

MaikKlein commented Sep 5, 2017 •
edited
Loading

I am currently trying to create a SPIR-V backend but I am running into a few problems.

For example https://github.com/rust-lang/rust/blob/master/src/librustc_trans/collector.rs is private https://github.com/rust-lang/rust/blob/master/src/librustc_trans/lib.rs#L108

It seems that a lot of useful functionality is inside collector.rs. Would it be possible to make some functionality public? I would be happy to provide a PR.
@eddyb
Copy link
Member

eddyb commented Sep 6, 2017

It should be moved to to librustc_mir. cc @nikomatsakis @michaelwoerister

@michaelwoerister
Copy link
Member

I think we'll need to turn it into a query soon anyway.

@eddyb
Copy link
Member

eddyb commented Sep 6, 2017

That doesn't help, IMO. It should be a reusable piece of code. librustc_trans is the LLVM backend and therefore non-reusable.

@TimNN TimNN added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 17, 2017
@MaikKlein
Copy link
Contributor Author

MaikKlein commented Oct 14, 2017 •
edited
Loading

I talked with eddyb on irc about this issue.

  • moving the collector to src/librustc_mir/monomorphize/collector.rs
  • changing terminology from "trans collector" to "monomorphization collector"

I am happy to do a PR myself.

@MaikKlein MaikKlein mentioned this issue Oct 25, 2017
Merged
bors added a commit that referenced this issue Dec 19, 2017
@bors
Auto merge of #45525 - MaikKlein:collector, r=eddyb
b76f224 
Move collector to librustc_mir::monomorphize

cc #44334 and #45276

* I moved the collector to rustc_mir

*  I renamed `TransItem` to `MonoItem`. _(I still need to fix up comments and variable names)_

* I got rid of `common.rs` and `monomorphize.rs` from `librustc_trans_utils`. I moved most of the functionality into `TyCtxt`. I realized that the `librustc_trans_utils::common.rs` was just copy pasted from `librustc_trans::common.rs`.

Should I also get rid of the `librustc_trans::common.rs` in this PR? Most of the functionality seems a bit useless, I decided to put some of it into `TyCtxt` but maybe that is not the correct action here.

Should I also get rid of `librustc_trans_utils` completely here? Or should I do it in a separate PR?
@MaikKlein
Copy link
Contributor Author

Resolved by #45525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@eddyb @TimNN @michaelwoerister @MaikKlein