Refactor rustc_trans::save to allow other backends than CSV #31838
Conversation
|
Update: I think I know the cause of the ICE. I hope to fix it tomorrow. |
|
Hmmm... it seems that it will be more difficult to find the bug than I had thought. I'll post here when I fix it. |
| use std::hash::Hasher; | ||
| use syntax::ast::{self, NodeId}; | ||
| use syntax::codemap::Span; | ||
|
|
There was a problem hiding this comment.
Can you add a comment to the file stating what is here? And perhaps move normalize_node_id lower in the file, since it is a utility function
|
Nice! Another test you should run is the save-analysis test in run-make - you should ensure that you get the same output before and after this refactoring (the automated test only checks that we don't crash while making the output). |
|
Thanks for the feedback! I'll let you know when I have addressed your comments and fixed the remaining bugs. |
|
☔ The latest upstream changes (presumably #31979) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@nrc I think this is almost ready to be merged... I managed to remove |
|
@aochagavia hmm, yeah the |
|
@nrc I will try to explain the reasoning behind the Instances of The case of I think the real question here is: Could we just adapt the old |
|
@aochagavia I think we should change the fields in ImplData with type Option to Option, then everything is in the same form (and I was never super happy with the nested setup). Why do you use I think the problem here is that the nice API stores 'maybe' refs as Option, but the CSV representation has no such concept, so uses 0. A JSON representation should use null. So each serialisation needs a function Option -> T (where T is String for csv and json, but might be binary data for something) which serialises these 'maybe' values. That should probably be literally a function. That seems like not too big a thing to fix now. If you'd rather land now, I'd like to at least address the CRATE_ROOT_DEF_ID issue and to summarise this conversation in the FIXME comments. |
|
IIRC, |
|
@nrc I tried to change Unless you can see an easy solution for this, I woud like to land it now and figure it out later. |
|
@aochagavia I don't understand - it looks in the commit like you have done the replacement of Option to Option? When you dump the impl, you should be able to use the def if to lookup the self/trait data from the save context. If that needs some work, then happy to land now. |
|
r? @nrc |
Move rustc_trans::*Data to own module Add new data to rustc_trans::save Create a Dump trait implemented by structs that dump save analysis data Split dump_csv into a DumpVisitor and a CsvDumper
|
@bors: r+ |
|
📌 Commit 67395d8 has been approved by |
Refactor rustc_trans::save to allow other backends than CSV r? @nrc Things done: * Moved `(.*)Data` structs to an own module, so they can be imported easily (`use data::*`). * Created a `Dump` trait with callbacks for dumping items. * Refactored `DumpCsvVisitor` to use an implementor of `Dump` instead of dumping as CSV. Renamed it to `DumpVisitor`. * Created a `DumpCsv` struct that implements `Dump` and serializes items as CSV. I tried to extract some of the logic contained in `FmtStr` and `Recorder`, such as normalization of ids (I put it in `DumpVisitor`). I think it makes sense to provide the same information to other implementors of `Dump`, instead of normalizing only for `DumpCsv`. However, there is still some logic related to spans implemented only for `DumpCsv`. I just thought it would be better to merge this as soon as possible, since there are so much changes, and fix this afterwards.
|
💔 Test failed - auto-win-gnu-64-opt |
|
@bors: retry |
r? @nrc
Things done:
(.*)Datastructs to an own module, so they can be imported easily (use data::*).Dumptrait with callbacks for dumping items.DumpCsvVisitorto use an implementor ofDumpinstead of dumping as CSV. Renamed it toDumpVisitor.DumpCsvstruct that implementsDumpand serializes items as CSV.I tried to extract some of the logic contained in
FmtStrandRecorder, such as normalization of ids (I put it inDumpVisitor). I think it makes sense to provide the same information to other implementors ofDump, instead of normalizing only forDumpCsv. However, there is still some logic related to spans implemented only forDumpCsv. I just thought it would be better to merge this as soon as possible, since there are so much changes, and fix this afterwards.