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

Code clean up to remove starboard/common/atomic usage #3944

Merged

Conversation

iuriionishchenko
Copy link
Collaborator

@iuriionishchenko iuriionishchenko commented Aug 7, 2024

Code clean up to remove starboard/common/atomic usage

Test-On-Device: true

b/353387554

@iuriionishchenko iuriionishchenko force-pushed the Clean_up_starboard_common_atomic branch 2 times, most recently from c4a9908 to 6901f77 Compare August 9, 2024 07:24
@iuriionishchenko iuriionishchenko force-pushed the Clean_up_starboard_common_atomic branch 3 times, most recently from 40c1d8e to b9b7a82 Compare August 9, 2024 11:11
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

I've reviewed, and changes look correct and good.

However - lets please take this opportunity to also consistently and correctly default-initialize the variables affected by this change.

It seems that the used style is generally std::atomic_var foo{value}; so let's use that everywhere for in-class initialization.

Of course, please doublecheck that the values match the expectation in code as well - in most cases atomic_bool is expected to be default false, but not always.

@iuriionishchenko iuriionishchenko force-pushed the Clean_up_starboard_common_atomic branch 2 times, most recently from c23c6d6 to edb1228 Compare August 12, 2024 12:29
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

lgtm now - please wait until internal builds come back green and Hao has a chance to review as well.

@iuriionishchenko
Copy link
Collaborator Author

There are 5 failing and 1 errored checks. But I don't have access to full logs to investigate and fix them. Can you provide me?

@kaidokert
Copy link
Member

There are 5 failing and 1 errored checks.

With on-device tests those are often flaky failures on the first try. When you see those, please click "Retry" on the failed jobs and see if one retry cleans them up. We don't currently auto-retry yet.

@haozheng-cobalt haozheng-cobalt enabled auto-merge (squash) August 14, 2024 17:44
@kaidokert
Copy link
Member

Hm there's lint failure from wrong include order - can we get that fixed please as well ?

b/353387554

Change-Id: I693905b5daac088c7e6e6acdb5e86f5feac4f3e5
auto-merge was automatically disabled August 15, 2024 09:47

Head branch was pushed to by a user without write access

@kaidokert kaidokert merged commit 24aee4e into youtube:main Aug 15, 2024
370 of 372 checks passed
kaidokert pushed a commit to kaidokert/cobalt that referenced this pull request Aug 15, 2024
Code clean up to remove starboard/common/atomic usage

Test-On-Device: true

b/353387554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants