Tidy and modernize SampleBuffer #11987

abique · 2023-09-15T16:23:00Z

No description provided.

JoergAtGithub · 2023-09-15T16:52:52Z

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

abique · 2023-09-15T16:55:22Z

Thank you!
I've signed it.
I'll update the PR with the style fix later tonight.

JoergAtGithub · 2023-09-15T17:01:28Z

Regarding the code style issues, we recommend to install pre-commit locally: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
Alternatively, you could download the patch file from the GitHub action and apply it manually.

abique · 2023-09-15T23:01:32Z

Unfortunately archlinux doesn't have yet the required version of pre-commit, so I'll use the patch for now.
I hope that clang-format does it right, so I'll use that in the mean time.

daschuer · 2023-09-16T07:18:46Z

Which pre-commit version do you have?

Did you try to Install pre-commit via pip?
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checkinng.

If you configure you IDE for clang-fotmat, make sure to reformat only touched lines!

src/util/assert.h

src/util/sample.h

src/util/samplebuffer.cpp

src/util/samplebuffer.h

src/util/samplebuffer.cpp

src/util/samplebuffer.h

abique · 2023-09-16T14:34:11Z

Which pre-commit version do you have?

Did you try to Install pre-commit via pip? https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checkinng.

If you configure you IDE for clang-fotmat, make sure to reformat only touched lines!

Archlinux has the x.20.x instead of the x.21.x and I've tried pip, but it wanted a system wide install, etc... so I didn't want to go that far.

Swiftb0y · 2023-09-16T14:35:58Z

have you tried installing pre-commit via pip in a venv?

abique · 2023-09-16T14:40:27Z

have you tried installing pre-commit via pip in a venv?

No, I'm short on disk space and I don't want to start hacking my system.
I'll wait for Archlinux to pickup the update.

Swiftb0y · 2023-09-16T14:45:12Z

Thats fine too, as long as you're fine with applying the pre-commit patches from CI for now. I'm confused why its not newer though. pre-commit 2.20.0 has been released more than a year ago and arch is a rolling distro, right? so it should've been picked up by now if there is not technical reason why it couldn't.

abique · 2023-09-16T14:47:28Z

Thats fine too, as long as you're fine with applying the pre-commit patches from CI for now. I'm confused why its not newer though. pre-commit 2.20.0 has been released more than a year ago and arch is a rolling distro, right? so it should've been picked up by now if there is not technical reason why it couldn't.

It is because the maintainer is probably lacking time to update the package.
I think that arch is working on allowing PR for updating packages, which could help maybe let's see.
In general it is quite up to date, but sometimes arch is behind.

abique · 2023-09-16T15:31:55Z

@daschuer @Swiftb0y thank you very much for the review.
I think we're good now.

Swiftb0y · 2023-09-16T18:02:38Z

Thank you. Can you please ensure pre-commit passes now? If you don't want / can't install it, you can find a patch file generated by CI in the pre-commit job. In this case https://github.com/mixxxdj/mixxx/actions/runs/6208187984

abique · 2023-09-16T18:58:17Z

Thank you. Can you please ensure pre-commit passes now? If you don't want / can't install it, you can find a patch file generated by CI in the pre-commit job. In this case https://github.com/mixxxdj/mixxx/actions/runs/6208187984

Yes I did apply this patch. However, it feels weird how much it indents the clobber list, feels like it has 1 extra level of indentation.

Swiftb0y · 2023-09-16T21:26:01Z

I assume you mean ConstructorInitializerIndentWidth: 8? Yeah that choice is weird but as far as I can tell its based on the style-guide from long before we used clang-format (or even before the majority of currently active devs even worked on mixxx...)

Swiftb0y

LGTM. Thank you. Your contributions are highly appreciated. waiting for LGTM from @daschuer

src/util/samplebuffer.h

Swiftb0y · 2023-09-18T10:49:35Z

src/util/samplebuffer.h

-    SampleBuffer()
-        : m_data(nullptr),
-          m_size(0) {
+    constexpr SampleBuffer()


Suggested change

constexpr SampleBuffer()

constexpr SampleBuffer() noexcept

Swiftb0y · 2023-09-18T10:51:34Z

src/util/samplebuffer.h

+            : m_data(std::exchange(that.m_data, nullptr)),
+              m_size(std::exchange(that.m_size, 0)) {
+    }
+    ~SampleBuffer() {


TIL destructors are noexcept by default since C++11, so even if we don't add it here, the destructor would std::terminate be default if SampleUtil::free would panic.

daschuer

We have now refined our rules regarding noexcept in https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#specifier-nexecept.
So here we have some extra noexecpt now. I will merge this anyway, because this PR has been started before. We can fix the style later.
Thank you @abique for the work!

abique · 2023-09-20T08:03:34Z

Hi,

Thanks for merging!

Sorry for all the noise with the annotations.

Cheers,
Alex.

Holzhaus · 2023-09-20T08:53:16Z

have you tried installing pre-commit via pip in a venv?

No, I'm short on disk space and I don't want to start hacking my system.
I'll wait for Archlinux to pickup the update.

Just wanted to let you know that I reached out to the package maintainer. The package was updated now.

abique · 2023-09-20T17:59:16Z

Just wanted to let you know that I reached out to the package maintainer. The package was updated now.

Yes I've seen that, thanks!

abique force-pushed the alex/tidy-sample-buffer branch from f273a11 to b81df8f Compare September 15, 2023 23:00

daschuer requested changes Sep 16, 2023

View reviewed changes

abique force-pushed the alex/tidy-sample-buffer branch 2 times, most recently from 1a083b6 to 2b65761 Compare September 16, 2023 13:52

abique requested a review from daschuer September 16, 2023 13:59

abique force-pushed the alex/tidy-sample-buffer branch from 2b65761 to b9f47e9 Compare September 16, 2023 14:03

Swiftb0y reviewed Sep 16, 2023

View reviewed changes

src/util/samplebuffer.cpp Outdated Show resolved Hide resolved

src/util/samplebuffer.cpp Outdated Show resolved Hide resolved

src/util/samplebuffer.h Outdated Show resolved Hide resolved

src/util/samplebuffer.h Show resolved Hide resolved

abique force-pushed the alex/tidy-sample-buffer branch from b9f47e9 to 2a3ad36 Compare September 16, 2023 14:28

abique force-pushed the alex/tidy-sample-buffer branch from 2a3ad36 to a9f12d1 Compare September 16, 2023 14:35

github-actions bot added the build label Sep 16, 2023

abique force-pushed the alex/tidy-sample-buffer branch from 50e9408 to 2f82efb Compare September 16, 2023 18:55

Swiftb0y approved these changes Sep 16, 2023

View reviewed changes

daschuer requested changes Sep 17, 2023

View reviewed changes

src/util/samplebuffer.h Show resolved Hide resolved

abique force-pushed the alex/tidy-sample-buffer branch from 2f82efb to d10e8c3 Compare September 18, 2023 08:59

Tidy and modernize SampleBuffer

1b29697

abique force-pushed the alex/tidy-sample-buffer branch from d10e8c3 to 1b29697 Compare September 18, 2023 10:41

Swiftb0y reviewed Sep 18, 2023

View reviewed changes

daschuer approved these changes Sep 19, 2023

View reviewed changes

daschuer merged commit c06c016 into mixxxdj:main Sep 19, 2023

abique deleted the alex/tidy-sample-buffer branch September 20, 2023 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy and modernize SampleBuffer #11987

Tidy and modernize SampleBuffer #11987

abique commented Sep 15, 2023

JoergAtGithub commented Sep 15, 2023

abique commented Sep 15, 2023

JoergAtGithub commented Sep 15, 2023

abique commented Sep 15, 2023

daschuer commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

Swiftb0y left a comment

Swiftb0y Sep 18, 2023

Swiftb0y Sep 18, 2023

daschuer left a comment

abique commented Sep 20, 2023

Holzhaus commented Sep 20, 2023

abique commented Sep 20, 2023

Tidy and modernize SampleBuffer #11987

Tidy and modernize SampleBuffer #11987

Conversation

abique commented Sep 15, 2023

JoergAtGithub commented Sep 15, 2023

abique commented Sep 15, 2023

JoergAtGithub commented Sep 15, 2023

abique commented Sep 15, 2023

daschuer commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

abique commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y Sep 18, 2023

Choose a reason for hiding this comment

Swiftb0y Sep 18, 2023

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

abique commented Sep 20, 2023

Holzhaus commented Sep 20, 2023

abique commented Sep 20, 2023