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

Store idents for DefPathData into crate metadata #70077

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Store idents for DefPathData into crate metadata #70077

merged 2 commits into from
Mar 24, 2020

Conversation

Aaron1011
Copy link
Member

Previously, we threw away the Span associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.

We now store the identifier's Span in a side table, which gets encoded
into the crate metadata. When we decode items from the metadata, we
combine the name and span back into an Ident.

This improves the output of several tests, which previously had messages
suppressed due to dummy spans.

This is a prerequisite for #68686, since throwing away a Span means
that we lose hygiene information.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2020
@Aaron1011 Aaron1011 mentioned this pull request Mar 17, 2020
Closed
@petrochenkov petrochenkov self-assigned this Mar 17, 2020
petrochenkov
petrochenkov reviewed Mar 18, 2020
View reviewed changes
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/decoder.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2020
@Aaron1011
Copy link
Member Author

@petrochenkov: Did you want to do a perf run for this?

@Aaron1011 Aaron1011 changed the title Store idents for DefPathData in a side table Store idents for DefPathData into crate metadata Mar 18, 2020
@bors

This comment has been minimized.

Sign in to view
@Aaron1011 Aaron1011 force-pushed the feature/new-def-path-ident branch from 50d5890 to 3760e40 Compare March 19, 2020 04:01
@petrochenkov
Copy link
Contributor

@Aaron1011
Yes, it would be good to run perf on this PR before merging it.

petrochenkov
petrochenkov reviewed Mar 19, 2020
View reviewed changes
src/librustc_metadata/rmeta/decoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/decoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Sign in to view
@Aaron1011 Aaron1011 force-pushed the feature/new-def-path-ident branch from 3760e40 to b442340 Compare March 19, 2020 19:01
@Aaron1011
Copy link
Member Author

@petrochenkov: I've addressed your comments.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Mar 19, 2020
View reviewed changes
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/rmeta/encoder.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

An accidental submodule update breaks CI.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2020
@Aaron1011 Aaron1011 force-pushed the feature/new-def-path-ident branch from e7d4cae to 35fb1cb Compare March 20, 2020 01:55
@Aaron1011
Copy link
Member Author

@petrochenkov: Updated. If this looks good to you, I'll squash the commits.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2020
@petrochenkov
Copy link
Contributor

@bors try @rust-timer queue

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 22, 2020
@Dylan-DPC
Rollup merge of rust-lang#70077 - Aaron1011:feature/new-def-path-iden…
8ac0d71 
…t, r=petrochenkov

Store idents for `DefPathData` into crate metadata

Previously, we threw away the `Span` associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.

We now store the identifier's `Span` in a side table, which gets encoded
into the crate metadata. When we decode items from the metadata, we
combine the name and span back into an `Ident`.

This improves the output of several tests, which previously had messages
suppressed due to dummy spans.

This is a prerequisite for rust-lang#68686, since throwing away a `Span` means
that we lose hygiene information.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 22, 2020
Closed
@Aaron1011 Aaron1011 mentioned this pull request Mar 23, 2020
Closed
3 tasks
@Centril
Copy link
Contributor

Centril commented Mar 23, 2020

Failed in #70288 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2020
@Aaron1011
Store idents for DefPathData into crate metadata
96e2d03 
Previously, we threw away the `Span` associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.

We now store the identifier's `Span` in the crate metadata.
When we decode items from the metadata, we combine
the name and span back into an `Ident`.

This improves the output of several tests, which previously had messages
suppressed due to dummy spans.

This is a prerequisite for rust-lang#68686, since throwing away a `Span` means
that we lose hygiene information.
@Aaron1011
Copy link
Member Author

This appears to be another instance of #53081: apparently, there are platform-specific issues with printing sysroot spans, which is causing some platforms to not print the output expected by the tests.

@Aaron1011 Aaron1011 force-pushed the feature/new-def-path-ident branch from 1f13a4f to 86b8dea Compare March 23, 2020 06:05
@Aaron1011
Copy link
Member Author

@petrochenkov: I've ignored the failing tests on the relevant platforms. It's unfortunate that this is required, but I'd prefer not to block this PR on resolving #53081.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2020

📌 Commit 86b8dea has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
@Centril
Rollup merge of rust-lang#70077 - Aaron1011:feature/new-def-path-iden…
9c08474 
…t, r=petrochenkov

Store idents for `DefPathData` into crate metadata

Previously, we threw away the `Span` associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.

We now store the identifier's `Span` in a side table, which gets encoded
into the crate metadata. When we decode items from the metadata, we
combine the name and span back into an `Ident`.

This improves the output of several tests, which previously had messages
suppressed due to dummy spans.

This is a prerequisite for rust-lang#68686, since throwing away a `Span` means
that we lose hygiene information.
@Centril Centril mentioned this pull request Mar 24, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
@Centril
Rollup merge of rust-lang#70077 - Aaron1011:feature/new-def-path-iden…
dd8ef75 
…t, r=petrochenkov

Store idents for `DefPathData` into crate metadata

Previously, we threw away the `Span` associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.

We now store the identifier's `Span` in a side table, which gets encoded
into the crate metadata. When we decode items from the metadata, we
combine the name and span back into an `Ident`.

This improves the output of several tests, which previously had messages
suppressed due to dummy spans.

This is a prerequisite for rust-lang#68686, since throwing away a `Span` means
that we lose hygiene information.
@Centril Centril mentioned this pull request Mar 24, 2020
Closed
@Centril Centril mentioned this pull request Mar 24, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2020
@bors
Auto merge of rust-lang#70351 - Centril:rollup-tveoq3w, r=Centril
9d0ae58 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#68884 (Make the `type_of` return a generic type for generators)
 - rust-lang#69788 (Fix sequence of Type and Trait in optin-builtin-traits in Unstable Book)
 - rust-lang#70074 (Expand: nix all fatal errors)
 - rust-lang#70077 (Store idents for `DefPathData` into crate metadata)
 - rust-lang#70213 (traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).)
 - rust-lang#70259 (Use Reveal::All in MIR optimizations)
 - rust-lang#70284 (correctly handle const params in type_of)
 - rust-lang#70289 (Refactor `codegen`)

Failed merges:

r? @ghost
@bors bors merged commit d626f5b into rust-lang:master Mar 24, 2020
@Aaron1011 Aaron1011 deleted the feature/new-def-path-ident branch March 24, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@eddyb eddyb eddyb left review comments

Assignees

@eddyb eddyb

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Aaron1011 @rust-highfive @bors @petrochenkov @rust-timer @Centril @eddyb