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

fix: increase IL2CPPP source mapping boundary #776

Merged
merged 3 commits into from
Mar 29, 2023
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Mar 27, 2023

In response to getsentry/sentry-unity#1239 and after manually sampling through some generated source files, this PR increases the boundary on the lines that are considered matching. From what I've seen so far, it should not increase false positive matches (significantly) but it should provide much better result for actual code that often spans ~30 lines for a block of source_info comments.

Closes getsentry/sentry-unity#1239

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #776 (3c58d3e) into master (657800f) will increase coverage by 0.29%.
The diff coverage is 98.88%.

❗ Current head 3c58d3e differs from pull request most recent head d1dcd4e. Consider uploading reports for the commit d1dcd4e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
+ Coverage   74.70%   74.99%   +0.29%     
==========================================
  Files          71       71              
  Lines       15416    15561     +145     
==========================================
+ Hits        11516    11670     +154     
+ Misses       3900     3891       -9     

@vaind vaind changed the title fix: il2cpp source mapping without boundary fix: increase IL2CPPP source mapping boundary Mar 27, 2023
@vaind vaind marked this pull request as ready for review March 27, 2023 10:44
@vaind vaind requested a review from a team March 27, 2023 10:44
let cpp_lines = cpp_source.split('\n').collect::<Vec<_>>();
let clean_cpp_lines = cpp_lines
.iter()
.map(|line| line.split_once('|').unwrap().0.trim_end())
Copy link
Member

Choose a reason for hiding this comment

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

there is also rsplit_once here and below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on a second look, I've restructured this part. It came together iteratively and could have been written cleaner, so I did that now.

assert_eq!(parsed_mapping.lookup("main.cpp", 5), Some(("main.cs", 17)));
assert_eq!(parsed_mapping.lookup("main.cpp", 12), None);
assert_eq!(parsed_mapping.lookup("main.cpp", 6), Some(("main.cs", 17)));
Copy link
Member

Choose a reason for hiding this comment

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

this might be a bit overkill here to enumerate every line. the important one I guess is that 12 now has a mapping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it may be but it also makes the expectation clear (and removes the guesswork) so if you don't mind I'd keep it there for each line

@vaind vaind force-pushed the fix/il2cpp-mapping branch 3 times, most recently from f25e915 to 0e2ac01 Compare March 29, 2023 10:55
@Swatinem Swatinem merged commit a8a755c into master Mar 29, 2023
@Swatinem Swatinem deleted the fix/il2cpp-mapping branch March 29, 2023 12:59
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.

IL2CPP stacktrace showing C++ instead of C# line numbers
2 participants