-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: Portable PDB source files list #729
Conversation
symbolic-debuginfo/src/ppdb.rs
Outdated
self.row += 1; | ||
|
||
let document = self.ppdb.get_document(index).ok()?; | ||
self.names[index] = document.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. I think you would have to append the name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here, why I was trying what I did in this original version you've reviewed, was that FileInfo
required reference - it didn't own the data. PPDB get_document() on the other hand produces a String
. This was incompatible and due to lifetime restrictions, there was no level in the PPDB API to produce correct FileInfo values whose lifetime could be based on 'data
. Therefore, after discussing with @Swatinem, I had to refactor FileInfo (and FileEntry, although that wasn't strictly necessary in this case) to use Cow
. This is a breaking change if anyone is constructing FileInfo/FileEntry
directly or reads its public properties instead of getters.
8a6e302
to
ad08495
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #729 +/- ##
==========================================
+ Coverage 72.76% 72.93% +0.17%
==========================================
Files 70 69 -1
Lines 14747 14726 -21
==========================================
+ Hits 10731 10741 +10
+ Misses 4016 3985 -31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Pls also add a changelog entry for this :-) |
My bad, forgot about that before merging |
"Alternative 2" of #725