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

ref: convert last sprintf call to snprintf #3077

Merged
merged 2 commits into from
May 31, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented May 31, 2023

📜 Description

Follows #2866 to convert potentially unsafe usage of sprintf to snprintf. See #2785.

💡 Motivation and Context

This also causes Xcode 13+ builds to break when using address sanitizer after making the changes in #3075

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
    - [ ] I added tests to verify the changes. The tests required for this would require extensive effort disproportionate to the simplicity of the change.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
    - [ ] I updated the docs if needed.
    - [ ] Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link

github-actions bot commented May 31, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1238.90 ms 1249.04 ms 10.14 ms
Size 20.76 KiB 435.24 KiB 414.47 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b9b0f0a 1257.22 ms 1262.57 ms 5.35 ms
6e342ac 1216.02 ms 1232.88 ms 16.86 ms
4c00f8c 1231.62 ms 1237.76 ms 6.14 ms
cbf6225 1221.73 ms 1251.20 ms 29.47 ms
98a8c16 1243.33 ms 1257.86 ms 14.53 ms
861d361 1227.90 ms 1231.45 ms 3.55 ms
9faf217 1268.86 ms 1274.82 ms 5.96 ms
438e21a 1237.47 ms 1255.24 ms 17.77 ms
c6773e5 1222.48 ms 1240.02 ms 17.54 ms
d253cdf 1231.61 ms 1259.52 ms 27.91 ms

App size

Revision Plain With Sentry Diff
b9b0f0a 20.76 KiB 434.94 KiB 414.17 KiB
6e342ac 20.76 KiB 436.66 KiB 415.90 KiB
4c00f8c 20.76 KiB 419.62 KiB 398.86 KiB
cbf6225 20.76 KiB 425.77 KiB 405.00 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
861d361 20.76 KiB 435.65 KiB 414.89 KiB
9faf217 20.76 KiB 419.70 KiB 398.94 KiB
438e21a 20.76 KiB 434.62 KiB 413.86 KiB
c6773e5 20.76 KiB 435.25 KiB 414.49 KiB
d253cdf 20.76 KiB 427.66 KiB 406.90 KiB

Previous results on branch: armcknight/ref/remaining-sprintf-conversion

Startup times

Revision Plain With Sentry Diff
cf52c93 1210.69 ms 1234.22 ms 23.53 ms

App size

Revision Plain With Sentry Diff
cf52c93 20.76 KiB 435.24 KiB 414.47 KiB

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #3077 (5b92446) into main (f587451) will increase coverage by 0.003%.
The diff coverage is 0.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3077       +/-   ##
=============================================
+ Coverage   88.835%   88.839%   +0.003%     
=============================================
  Files          493       493               
  Lines        53135     53141        +6     
  Branches     19034     19034               
=============================================
+ Hits         47203     47210        +7     
  Misses        4970      4970               
+ Partials       962       961        -1     
Impacted Files Coverage Δ
Sources/SentryCrash/Recording/SentryCrashReport.c 34.267% <0.000%> (ø)

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f587451...5b92446. Read the comment docs.

@brustolin
Copy link
Contributor

Validate XCFramework will be fixed once we merge #3075

@armcknight armcknight merged commit c78683b into main May 31, 2023
@armcknight armcknight deleted the armcknight/ref/remaining-sprintf-conversion branch May 31, 2023 22:33
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.

2 participants