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

Make reprs use a structured representation instead of a slice #39595

Merged
merged 2 commits into from Feb 9, 2017

Conversation

ahicks92
Copy link
Contributor

@ahicks92 ahicks92 commented Feb 6, 2017

This is needed for -z reorder-fields. The old design uses a slice taken from HIR, plus a cache that lazily parses. The new design stores it directly in the AdtDef as a ReprOptions. We're doing this now because we need to be able to add reprs that don't necessarily exist in HIR for -z reorder-fields, but it needs to happen anyway.

lookup_repr_hints should be mostly deprecated. I want to remove it from layout before closing this, unless people think that should be a separate PR. The [WIP] is because of this. The problem with closing this as-is is that the code here isn't actually testable until some parts of the compiler start using it.

r? @eddyb

@@ -1818,6 +1847,14 @@ impl<'tcx> TyS<'tcx> {
}
}
}


Copy link
Member

@eddyb eddyb Feb 6, 2017

Choose a reason for hiding this comment

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

Extraneous empty line.

@@ -1818,6 +1847,14 @@ impl<'tcx> TyS<'tcx> {
}
}
}


pub fn get_repr(&self) -> Option<&ReprOptions> {
Copy link
Member

@eddyb eddyb Feb 6, 2017

Choose a reason for hiding this comment

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

This is only used in encoder.rs, could it be a helper method there?

Copy link
Contributor Author

@ahicks92 ahicks92 Feb 6, 2017

Choose a reason for hiding this comment

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

Yeah, it could. But my thought is that things like future lints are going to want it as well.

@@ -401,8 +401,12 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
}
}

let repr_options = *self.tcx.item_type(adt_def_id).get_repr().expect(
&format!("{:?} is not an ADT", def_id)
Copy link
Member

@eddyb eddyb Feb 6, 2017

Choose a reason for hiding this comment

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

Probably not the best to have this allocating here. If you make an encoding-specific helper method you can just use bug! in cases other than TyAdt.

Copy link
Contributor Author

@ahicks92 ahicks92 Feb 6, 2017

Choose a reason for hiding this comment

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

I'll factor those out.

@ahicks92
Copy link
Contributor Author

@ahicks92 ahicks92 commented Feb 7, 2017

I've moved all of this to a helper method in encoder.rs for now. We can introduce something more general later if needed.

…utes. This effectively deprecates lookup_repr_hints.
@ahicks92
Copy link
Contributor Author

@ahicks92 ahicks92 commented Feb 7, 2017

Unless we want me to try to kill more lookup_repr_hints calls, I think this is at a mergable point. it should at least be sufficient for -z reorder-fields if nothing else.

if someone wants to suggest a large crate with a lot of tests that I can build really quickly to see if metadata is actually working, that's not necessarily a bad idea.

@eddyb
Copy link
Member

@eddyb eddyb commented Feb 7, 2017

@bors r+

@bors
Copy link
Contributor

@bors bors commented Feb 7, 2017

📋 Looks like this PR is still in progress, ignoring approval

@eddyb eddyb changed the title [WIP] Make reprs use a structured representation instead of a slice Make reprs use a structured representation instead of a slice Feb 7, 2017
@eddyb
Copy link
Member

@eddyb eddyb commented Feb 7, 2017

@bors r+

@bors
Copy link
Contributor

@bors bors commented Feb 7, 2017

📌 Commit c3b64cf has been approved by eddyb

bors added a commit that referenced this issue Feb 8, 2017
Rollup of 6 pull requests

- Successful merges: #38165, #39456, #39587, #39589, #39598, #39641
- Failed merges: #39586, #39595
bors added a commit that referenced this issue Feb 9, 2017
Rollup of 7 pull requests

- Successful merges: #37928, #39456, #39587, #39589, #39641, #39649, #39662
- Failed merges: #39586, #39595
bors added a commit that referenced this issue Feb 9, 2017
Rollup of 7 pull requests

- Successful merges: #37928, #39456, #39587, #39589, #39641, #39649, #39662
- Failed merges: #39586, #39595
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 9, 2017
Make reprs use a structured representation instead of a slice

This is needed for `-z reorder-fields`.  The old design uses a slice taken from HIR, plus a cache that lazily parses.  The new design stores it directly in the `AdtDef` as a `ReprOptions`.  We're doing this now because we need to be able to add reprs that don't necessarily exist in HIR for `-z reorder-fields`, but it needs to happen anyway.

`lookup_repr_hints` should be mostly deprecated.  I want to remove it from `layout` before closing this, unless people think that should be a separate PR.  The `[WIP]` is because of this.  The problem with closing this as-is is that the code here isn't actually testable until some parts of the compiler start using it.

r? @eddyb
bors added a commit that referenced this issue Feb 9, 2017
Rollup of 5 pull requests

- Successful merges: #39595, #39601, #39602, #39615, #39647
- Failed merges:
@bors bors merged commit c3b64cf into rust-lang:master Feb 9, 2017
1 check passed
@ahicks92 ahicks92 deleted the structured_repr branch Feb 9, 2017
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

3 participants