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

Move variant_size_differences out of trans #34755

Merged
merged 5 commits into from
Jul 12, 2016
Merged

Move variant_size_differences out of trans #34755

merged 5 commits into from
Jul 12, 2016

Conversation

jonas-schievink
Copy link
Contributor

Also enhances the error message a bit, fixes #30505 on the way, and adds
a test (which was missing).

Closes #34018

Also enhances the error message a bit, fixes #30505 on the way, and adds
a test (which was missing).

Closes #34018
@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@nrc
Copy link
Member

nrc commented Jul 10, 2016

r? @eddyb (I'm so out of date with trans nowadays)

@rust-highfive rust-highfive assigned eddyb and unassigned nrc Jul 10, 2016
let lvlsrc = cx.lints.get_level_source(lint_id);
match lvlsrc {
(lvl, _) if lvl != Allow => {
cx.node_levels.borrow_mut()
Copy link
Member

Choose a reason for hiding this comment

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

Does anything else use node_levels? Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't look like it, I'll get rid of this then 🎉

@eddyb
Copy link
Member

eddyb commented Jul 10, 2016

r=me with comments answered/resolved.

No need to store all sizes in a vector
Presumably, this ICEs when translating an inlined item from another
crate. There shouldn't be a need to track dependencies in that case.
@jonas-schievink
Copy link
Contributor Author

Comments addressed. The last commit fixes a bug where I tried to register a trans task for items from other crates (should also fix Travis). Am I correct in assuming that this doesn't need any dependency tracking?

@eddyb
Copy link
Member

eddyb commented Jul 11, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 11, 2016

📌 Commit 37d5c06 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 11, 2016

⌛ Testing commit 37d5c06 with merge 278c803...

@bors
Copy link
Contributor

bors commented Jul 11, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@jonas-schievink
Copy link
Contributor Author

Apparently trans dep tracking was only supposed to be used for items, not impl items? The failure happens because I'm trying to register reads on an impl item (a drop method), and the dep graph logic uses expect_item when getting saved, which then ICEs.

Should trans dependencies of methods not get tracked in the dep graph?

@michaelwoerister
Copy link
Member

Should trans dependencies of methods not get tracked in the dep graph?

Yes, at the moment only track things at the HIR item level, which has the somewhat surprising consequence that we do not track single methods, only entire impls. This will probably change at some point.

I'll have a closer at the dependency tracking side of this PR shortly.

@michaelwoerister
Copy link
Member

This last commit looks good to me!

@jonas-schievink
Copy link
Contributor Author

...and Travis is happy, too!

@michaelwoerister
Copy link
Member

michaelwoerister commented Jul 11, 2016

From my point of view the dependency tracking changes in this PR are OK. I've opened issue #34765 to remind us of cleanup that should be done later on.

@eddyb The rest still good to merge?

@eddyb
Copy link
Member

eddyb commented Jul 11, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 11, 2016

📌 Commit fd2b65e has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 12, 2016

⌛ Testing commit fd2b65e with merge 5c69a4f...

bors added a commit that referenced this pull request Jul 12, 2016
Move variant_size_differences out of trans

Also enhances the error message a bit, fixes #30505 on the way, and adds
a test (which was missing).

Closes #34018
@bors bors merged commit fd2b65e into rust-lang:master Jul 12, 2016
@jonas-schievink jonas-schievink deleted the minor-differences branch July 12, 2016 13:04
@michaelwoerister
Copy link
Member

🎉

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.

Move enum_variant_size_lint out of trans_crate(). Remove -Zprint_enum_sizes
6 participants