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.

Sign up for GitHub

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

Jump to bottom

Initial pass of removing scoped_ptr/scoped_array #1163

Closed
wants to merge 1 commit into from
Closed

Initial pass of removing scoped_ptr/scoped_array #1163

wants to merge 1 commit into from

Conversation

kaidokert
Copy link
Member

@kaidokert kaidokert commented Aug 6, 2023 •
edited
Loading

Switches to use unique_ptr instead of scoped_ptr where possible and it doesn't break tests.

The intent is to remove starboard/common/scoped_ptr fully.

b/291356560

@kaidokert kaidokert force-pushed the scoped_ptr_phase_1 branch 3 times, most recently from 3445026 to 91785b3 Compare August 6, 2023 04:21
@codecov
Copy link

codecov bot commented Aug 6, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 53.22581% with 29 lines in your changes missing coverage. Please review.

Project coverage is 56.53%. Comparing base (ef26918) to head (f57c2c0).
Report is 2313 commits behind head on main.

Files with missing lines Patch % Lines
starboard/nplb/socket_helpers.cc 60.60% 13 Missing ⚠️
starboard/shared/starboard/link_receiver.cc 47.61% 11 Missing ⚠️
starboard/shared/x11/application_x11.cc 0.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1163      +/-   ##
==========================================
+ Coverage   56.52%   56.53%   +0.01%     
==========================================
  Files        1899     1899              
  Lines       94249    94307      +58     
==========================================
+ Hits        53270    53317      +47     
- Misses      40979    40990      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaidokert kaidokert force-pushed the scoped_ptr_phase_1 branch 5 times, most recently from e4faf01 to 08c62bc Compare August 6, 2023 07:52
@kaidokert kaidokert force-pushed the scoped_ptr_phase_1 branch from 08c62bc to 65fe06f Compare August 6, 2023 19:02
@kaidokert kaidokert requested a review from joeltine August 6, 2023 19:18
kaidokert added a commit to kaidokert/cobalt that referenced this pull request Aug 6, 2023
@kaidokert
Media scoped_ptr/scoped_array switch
9d51f35 
Note this is a draft, depends on youtube#1163

b/291356560
@kaidokert kaidokert mentioned this pull request Aug 6, 2023
Closed
@kaidokert kaidokert force-pushed the scoped_ptr_phase_1 branch 2 times, most recently from eee2561 to 8ae4601 Compare August 9, 2023 03:35
@kaidokert
Initial pass of removing scoped_ptr/scoped_array
f57c2c0 
Switches to use unique_ptr instead of scoped_ptr
where possible and it doens't break tests.

The intent is to remove
starboard/common/scoped_ptr fully.

b/291356560
joeltine
joeltine requested changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@joeltine joeltine left a comment

Choose a reason for hiding this comment

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

LGTM but can you tell me if the current test failures are legitimate?

@kaidokert
Copy link
Member Author

LGTM but can you tell me if the current test failures are legitimate?

For internal builds they are yes. Haven't had time to debug - i've been backing changes out one by one. External builds are ( were ? ) fine, except that the lab has been down for couple days for on-device tests.

@joeltine
Copy link
Contributor

For internal builds they are yes. Haven't had time to debug - i've been backing changes out one by one. External builds are ( were ? ) fine, except that the lab has been down for couple days for on-device tests.

OK LMK when tests are passing.

@xiaomings xiaomings removed their request for review November 7, 2023 17:50
@kaidokert
Copy link
Member Author

This is now worked on in #2427

Conveerting to a draft

@kaidokert kaidokert marked this pull request as draft February 13, 2024 18:05
@kaidokert kaidokert closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeltine joeltine joeltine requested changes

@jellefoks jellefoks jellefoks approved these changes

Assignees
No one assigned
Labels
on_device
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaidokert @joeltine @jellefoks