Skip to content

feat: Portable PDB source files list#729

Merged
loewenheim merged 6 commits into
getsentry:masterfrom
vaind:feat/ppdb-source-files
Dec 6, 2022
Merged

feat: Portable PDB source files list#729
loewenheim merged 6 commits into
getsentry:masterfrom
vaind:feat/ppdb-source-files

Conversation

@vaind

@vaind vaind commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

"Alternative 2" of #725

Comment thread symbolic-debuginfo/src/ppdb.rs Outdated
Comment thread symbolic-debuginfo/src/ppdb.rs Outdated
self.row += 1;

let document = self.ppdb.get_document(index).ok()?;
self.names[index] = document.name;

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

Comment thread symbolic-ppdb/src/format/mod.rs Outdated
@vaind vaind force-pushed the feat/ppdb-source-files branch from 8a6e302 to ad08495 Compare December 5, 2022 20:40
@vaind vaind marked this pull request as ready for review December 5, 2022 20:40
@vaind vaind requested a review from a team December 5, 2022 20:40
@codecov-commenter

codecov-commenter commented Dec 5, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.75510% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.93%. Comparing base (0b39652) to head (903a645).
⚠️ Report is 271 commits behind head on master.

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     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loewenheim loewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice!

Comment thread symbolic-debuginfo/src/base.rs Outdated
@loewenheim loewenheim merged commit 21295ef into getsentry:master Dec 6, 2022
@Swatinem

Swatinem commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

Pls also add a changelog entry for this :-)

@loewenheim

Copy link
Copy Markdown
Contributor

My bad, forgot about that before merging

loewenheim added a commit that referenced this pull request Dec 6, 2022
Swatinem pushed a commit that referenced this pull request Dec 6, 2022
@vaind vaind deleted the feat/ppdb-source-files branch December 12, 2022 11:00
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.

4 participants