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

save-analysis: use a decoupled representation for dumped data #33370

Merged
merged 4 commits into from
May 10, 2016
Merged

save-analysis: use a decoupled representation for dumped data #33370

merged 4 commits into from
May 10, 2016

Conversation

aochagavia
Copy link
Contributor

Closes #33348

This will probably break any tool relying on the csv backend of save_analysis, for the following reasons:

  1. Dumped spans don't contain extents anymore (Dump uses SpanData now instead of internal Spans). In case we still want to dump extents we could add them to SpanData.
  2. DefIds are no longer dumped as a pair of (ref_id, ref_crate). Instead, they are dumped as a single Id.

@nrc You said something about storing the id in a u64, but you didn't explain why. I kept using u32 in this branch but I can change it if you prefer that.

r? @nrc

By the way, the fact that this breaks tools relying on CSV may be a good occasion to start dumping CSV in a different way (i.e. using the serializer like in the JSON backend).

@nrc
Copy link
Member

nrc commented May 4, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 4, 2016

📌 Commit dca29d7 has been approved by nrc

@nrc
Copy link
Member

nrc commented May 6, 2016

@bors: r-

@nrc
Copy link
Member

nrc commented May 6, 2016

@aochagavia so I was thinking some more about ids, and I think what I started and which is continued here is bogus.

By using the index from the def_id and ignoring the crate num, we could have duplicate ids where two items have the same index, but are in different crates. My plan was to fix that by using a u64, where the low 32 bits would be the index and the high would be the crate num, but this doesn't work as an actual id - the client still has to do some work to convert local ids (i.e., those with crate num = 0) into non-local ones and to normalise crate numbers between crates. I feel like this is less obvious in a single id field, rather than having two fields for a def id, i.e., we're getting worse, not better. As far as I can see, there is no way to do this normalisation in the compiler and we must leave it to the client.

Sorry I didn't think of this earlier.

@aochagavia
Copy link
Contributor Author

@nrc so the crux of the problem is normalizing crate numbers between crates. However, if the compiler itself doesn't have enough information to do it, how can we expect the clients to do it?

@nrc
Copy link
Member

nrc commented May 8, 2016

@aochagavia Once a tool has the crate dump for every crate, then a tool has all the info it needs to do the normalisation (this is what DXR does). The problem for the compiler is that it only has information on the current crate (and sort of earlier crates), but not any crates being compiler later.

Did you see my PR against this PR? https://github.com/aochagavia/rust/pull/1

@aochagavia
Copy link
Contributor Author

@nrc I just merged your PR :)

@nrc
Copy link
Member

nrc commented May 9, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 9, 2016

📌 Commit 192e336 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 9, 2016

⌛ Testing commit 192e336 with merge 72cfc12...

@bors
Copy link
Contributor

bors commented May 9, 2016

💔 Test failed - auto-win-msvc-64-opt

@nrc
Copy link
Member

nrc commented May 9, 2016

@bors: retry

Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016
save-analysis: use a decoupled representation for dumped data

Closes rust-lang#33348

This will probably break any tool relying on the csv backend of save_analysis, for the following reasons:

1. Dumped spans don't contain extents anymore (`Dump` uses `SpanData` now instead of internal `Span`s). In case we still want to dump extents we could add them to `SpanData`.
1. `DefId`s are no longer dumped as a pair of `(ref_id, ref_crate)`. Instead, they are dumped as a single `Id`.

@nrc You said something about storing the id in a `u64`, but you didn't explain why. I kept using `u32` in this branch but I can change it if you prefer that.

r? @nrc

By the way, the fact that this breaks tools relying on CSV may be a good occasion to start dumping CSV in a different way (i.e. using the serializer like in the JSON backend).
Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016
save-analysis: use a decoupled representation for dumped data

Closes rust-lang#33348

This will probably break any tool relying on the csv backend of save_analysis, for the following reasons:

1. Dumped spans don't contain extents anymore (`Dump` uses `SpanData` now instead of internal `Span`s). In case we still want to dump extents we could add them to `SpanData`.
1. `DefId`s are no longer dumped as a pair of `(ref_id, ref_crate)`. Instead, they are dumped as a single `Id`.

@nrc You said something about storing the id in a `u64`, but you didn't explain why. I kept using `u32` in this branch but I can change it if you prefer that.

r? @nrc

By the way, the fact that this breaks tools relying on CSV may be a good occasion to start dumping CSV in a different way (i.e. using the serializer like in the JSON backend).
bors added a commit that referenced this pull request May 9, 2016
Rollup of 10 pull requests

- Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514
- Failed merges:
Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016
save-analysis: use a decoupled representation for dumped data

Closes rust-lang#33348

This will probably break any tool relying on the csv backend of save_analysis, for the following reasons:

1. Dumped spans don't contain extents anymore (`Dump` uses `SpanData` now instead of internal `Span`s). In case we still want to dump extents we could add them to `SpanData`.
1. `DefId`s are no longer dumped as a pair of `(ref_id, ref_crate)`. Instead, they are dumped as a single `Id`.

@nrc You said something about storing the id in a `u64`, but you didn't explain why. I kept using `u32` in this branch but I can change it if you prefer that.

r? @nrc

By the way, the fact that this breaks tools relying on CSV may be a good occasion to start dumping CSV in a different way (i.e. using the serializer like in the JSON backend).
bors added a commit that referenced this pull request May 9, 2016
Rollup of 10 pull requests

- Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514
- Failed merges:
@bors bors merged commit 192e336 into rust-lang:master May 10, 2016
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.

3 participants