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

Audio clean-up and style/usage fixes #10115

Merged
merged 9 commits into from May 10, 2020

Conversation

tomlankhorst
Copy link
Contributor

  • use appropriate static casts
  • use appropriate dynamic cast
  • remove unused headers
  • mark 'pure' getters as 'nodiscard'
  • remove redundant virtual
  • mark override
  • default initialize some structs

@tomlankhorst
Copy link
Contributor Author

I might be making a mistake... but Travis' clang-format seems to be inconsistent with mine.

@AaronVanGeffen
Copy link
Contributor

Travis' clang-format seems to be inconsistent with mine.

We're probably still using an old version of clang-format. I think we have to manually update, and it's been a while.

@tomlankhorst
Copy link
Contributor Author

@AaronVanGeffen Seems @IntelOrca did the latest docker image update of openrct2/openrct2:format. Maybe he can run a docker build, docker push? Or even better, we automate it.

@janisozaur
Copy link
Member

That's not the right way to do. Build a static version of clang-format or use one already available in angular repos, don't build full OS just for that. I will eventually do that, once I have some more time.

@tomlankhorst
Copy link
Contributor Author

tomlankhorst commented Oct 21, 2019

@janisozaur Actually, building an image for a single executable is a typical use-case. And although the openrct2/openrct2:format includes many packages that are not used, the corresponding image layers will be reused from other openrct2/openrct2 images. It's not a full-blown OS in the end, it's just some namespacing with about 3 MB of supporting binaries (in the case of Alpine).

Take unibeautify/clang-format for example, it's just 33 MB in total.

@IntelOrca
Copy link
Contributor

IntelOrca commented Oct 21, 2019

don't build full OS just for that

OS implies a having a kernel right? That is no kernel building involved with docker. As @tomlankhorst pointed out, docker has always intended to be for running single apps, such as web apps.

@Gymnasiast
Copy link
Member

@tomlankhorst Do you plan to continue work on this PR?

@tomlankhorst tomlankhorst force-pushed the fix/audio-cleanup branch 2 times, most recently from 5456398 to 0dcc0ce Compare March 22, 2020 19:25
@tupaschoal
Copy link
Member

I was going to review, but the builds are failing, please take a look :)

@tomlankhorst
Copy link
Contributor Author

I was going to review, but the builds are failing, please take a look :)

Sorry for the delay. I try to fix it today.

@tupaschoal
Copy link
Member

No problem/rush, I just didn't know if you saw it, so I commented

@tupaschoal tupaschoal added this to the After v0.2.6 milestone Apr 13, 2020
@tupaschoal tupaschoal added the pending rebase PR needs to be rebased. label Apr 13, 2020
@tupaschoal tupaschoal modified the milestones: After v0.2.6, v0.2.7 Consideration Apr 17, 2020
@Gymnasiast Gymnasiast removed the pending rebase PR needs to be rebased. label Apr 24, 2020
@Gymnasiast
Copy link
Member

I think this should have been split into multiple parts. Some of the plain fixes (like [nodiscard]) are likely to be merged with very little discussion, while other things might take a long time.

Copy link
Member

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I think it's good to go with the suggestions

src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
src/openrct2-ui/audio/AudioMixer.cpp Outdated Show resolved Hide resolved
@tupaschoal tupaschoal merged commit 2d0e7bd into OpenRCT2:develop May 10, 2020
@Gymnasiast Gymnasiast added this to the v0.2.7 milestone May 10, 2020
rdbaris pushed a commit to rdbaris/OpenRCT2 that referenced this pull request Jun 7, 2020
* openrct2-ui Audio clean-up and style/usage fixes



Co-authored-by: Gymnasiast <m.o.steenbeek@gmail.com>
Co-authored-by: Tulio Leao <tupaschoal@gmail.com>
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.

None yet

7 participants