Cleanup and deprecate more `util/` classes #13687

Swiftb0y · 2024-09-23T12:04:48Z

This should conclude #13222 along with a bunch of drive by fixes I couldn't resist not to do.

* convert class to namespace * replace unecessary includes with forward decls * replace `NULL` and `0` with `nullptr`

acolombier

This a great cleanup! Thanks for looking into this.

The CI seems to be failing, can you look at this?

src/engine/controls/ratecontrol.cpp

acolombier · 2024-09-23T17:57:09Z

src/skin/legacy/legacyskinparser.cpp

- pTransformer = ValueTransformer::parseFromXml(transform, *m_pContext);
+ pTransformer =
+ ValueTransformer::parseFromXml(transform, *m_pContext)
+ .get(); // TODO don't leak here


TODO? Could it be possible to make pTransformer a smart pointer?

yes, but didn't want to here for the sake of keeping the scope small. Actually this is a bug, I want .release() here otherwise it'll be a double free...

Swiftb0y · 2024-09-23T19:14:54Z

The CI seems to be failing, can you look at this?

Yeah, I'll do so asap. though I'm afraid I won't have much time for mixxx soon.

daschuer · 2024-09-24T06:48:18Z

src/util/valuetransformer.cpp

+
+ double transformInverse(double argument) const override {
+ if (argument > 0.0) {
+ return m_compareValue;


Is that correct inversion of Equal? Maybe we should add a brief comment.

didn't bother checking, I just moved this part from the header into the cpp.

Swiftb0y · 2024-09-24T13:01:35Z

actually I just now realized that QQueue is terribly inefficient in terms of memory use, so that'll take a little while to rectify.

daschuer · 2024-09-25T15:22:55Z

src/util/compatibility/qatomic.h


+DEPRECATED_QATOMIC_OPS


What does this do?
Did you consider to fix the code instead?

It just copy-pastes the message above so it's shown during code completion. I could fix this (since main is Qt6 only), but that would be touching another 20 files which I tried to avoid. I can do it though if you prefer.

The preprocessor stops at comments. In my case Eclipse shows as macro expansion just an empty field.
So I think you should remove this and the comment all together. From the function content it is clear that the function is deprecated, because the main branch is on QT6.

It is OK to keep the current state with this wrapper since there is no user benefit to remove it.

I have removed the comment and #define

daschuer · 2024-09-26T07:31:57Z

src/util/rotary.h

- double m_dLastValue;
- int m_iCalibrationCount;
+ private:
+ QQueue<double> m_filterHistory;


QQueue is not effective for this prupose. I suggest to restore the std::vector<double> based solution.
The m_pFilter can be called m_pRingBuffer to make the use cas obvious.

yes, I did something similar in a follow up with I haven't pushed yet.

see 7578cfa

daschuer · 2024-09-26T07:39:06Z

src/util/rotary.cpp

- m_dLastValue(0.0),
- m_iCalibrationCount(0) {
- for (int i = 0; i < m_iFilterLength; ++i) {
- m_pFilter[i] = 0.;


Without that loop, the jog wheel is unfiltered for the first samples. This here assumes that the jog is not moving before the controller is initalized. Is it it a problem? The workarund was the removed fillBuffer() function.

I'm not sure. If the loop is pre-filled with zeros then the mean is heavily distorted until those zeros have been flushed.

Actually, thinking about this more, this could be the reason why I have had the issue for years where moving the jog suddenly would be slow at first and then really fast after a couple split seconds. I will do some testing to confirm, but now I'm fairly certain that filling the buffer is wrong!

daschuer · 2024-09-26T07:48:47Z

src/util/class.h

+// DEPRECATED: Instead rely on "the rule of zero". See
+// https://en.cppreference.com/w/cpp/language/rule_of_three#Rule_of_zero
+// See also: Cpp Core Guidelines C.20, C.21 & C.22 as well as the clang-tidy rule
+// `modernize-use-equals-delete`


I don't undestand the comment. The modernize-use-equals-delete explict cites this solution as good.
You are probably refering the old impmentation of the macro.

Suggested change

// DEPRECATED: Instead rely on "the rule of zero". See

// https://en.cppreference.com/w/cpp/language/rule_of_three#Rule_of_zero

// See also: Cpp Core Guidelines C.20, C.21 & C.22 as well as the clang-tidy rule

// `modernize-use-equals-delete`

// DEPRECATED: Instead delete functions explicit without using this macro.

well what I actually want to say that people should reconsider using this macro as per the rule of zero. There are two problems:

Its used all over the place (more than is necessary nor good).

It violates CppCoreGL C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all.

So the best practice IMO is that if a class deals with ownership it should be its sole purpose (I think we both agree on that). If it does that, it should adhere to the rule of zero and do that explicitly (not via this macro or by mixing this macro). Classes that don't deal with ownership should rely on implicitly generated SMF's and not delete them explicitly (for example by using this macro). Considering all that, this macro should not exist in code following best practices and thus I mark it deprecated.

The actual point however is not do it explicitly everywhere, but rather reconsider refactoring to follow the rule of zero, which is why I added all the references. So I don't agree with your suggested change because it does not link to the proper best practices.

The modernize-use-equals-delete explict cites this solution as good.

I wanted to mention it so people aren't tempted to do it the pre C++11 way.

OK, But than write an explicit advice what to do instead of this macro. A short for of your last comment?

Sure, I'll try my best.

…omic.h

Swiftb0y · 2024-10-10T21:50:33Z

src/util/rotary.h

+ Rotary(qsizetype filterLength)
+ : m_filterHistory(filterLength, 0.0) {


note, that this does restore the previous behavior. I have not yet experimented with more dynamic filtering.

Swiftb0y added 8 commits September 21, 2024 19:52

refactor: remove unused and unmaintained ThreadCpuTimer

b6696e9

refactor: slim down valuetransformer.h and auto memory management

ba9b7e8

refactor: fix some transient dependencies on util/math.h

534d6c4

refactor: convert static-only member classes to namespaces

152e344

refactor: slim down font.h

448ca20

refactor: remove unused atomic utility functions from util/qatomic.h

07877a1

refactor: remove util/math.h dependency from util/types.h

7849fa4

refactor: slim down util/xml.h

79e7506

* convert class to namespace * replace unecessary includes with forward decls * replace `NULL` and `0` with `nullptr`

Swiftb0y requested a review from acolombier September 23, 2024 12:04

github-actions bot added effects engine ui build preferences waveform analyzer labels Sep 23, 2024

acolombier reviewed Sep 23, 2024

View reviewed changes

daschuer reviewed Sep 24, 2024

View reviewed changes

daschuer reviewed Sep 25, 2024

View reviewed changes

daschuer reviewed Sep 26, 2024

View reviewed changes

Swiftb0y added 6 commits October 9, 2024 11:43

refactor: simplify Rotary (jog) filter drastically.

2109be2

chore: mark DISALLOW_COPY_AND_ASSIGN macro as deprecated

0e4dd46

refactor: cleanup SemanticVersion

923310d

refactor: modernize TaskWatcher and mark deprecated

ad841da

refactor: slim down trace.h

ec0c12d

refactor: make Autopan RampedSample constexpr, fix cmath abs

ec51089

refactor: optimize Rotary Jog filter (moving arithm. average)

7578cfa

Swiftb0y force-pushed the refactor/util-deprecate branch from 40c2240 to 7578cfa Compare October 9, 2024 09:44

Swiftb0y added 5 commits October 9, 2024 11:46

fixup! refactor: slim down valuetransformer.h and auto memory management

4100f30

fixup! refactor: remove unused atomic utility functions from util/qat…

0698a5b

…omic.h

fixup! chore: mark DISALLOW_COPY_AND_ASSIGN macro as deprecated

77798f1

fixup! refactor: cleanup SemanticVersion

0f69157

fixup! refactor: remove unused and unmaintained ThreadCpuTimer

c03c588

Swiftb0y commented Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup and deprecate more `util/` classes #13687

Cleanup and deprecate more `util/` classes #13687

Swiftb0y commented Sep 23, 2024

acolombier left a comment

acolombier Sep 23, 2024

Swiftb0y Sep 23, 2024 •

edited

Loading

Swiftb0y Oct 9, 2024

Swiftb0y commented Sep 23, 2024

daschuer Sep 24, 2024 •

edited

Loading

Swiftb0y Sep 24, 2024

Swiftb0y commented Sep 24, 2024

daschuer Sep 25, 2024

Swiftb0y Sep 25, 2024

daschuer Sep 25, 2024

Swiftb0y Oct 9, 2024

daschuer Sep 26, 2024

Swiftb0y Oct 4, 2024

Swiftb0y Oct 9, 2024

daschuer Sep 26, 2024

Swiftb0y Oct 4, 2024

Swiftb0y Oct 4, 2024

daschuer Sep 26, 2024

Swiftb0y Oct 4, 2024

daschuer Oct 4, 2024

Swiftb0y Oct 4, 2024

Swiftb0y Oct 9, 2024

Swiftb0y Oct 10, 2024

		Rotary(qsizetype filterLength)
		: m_filterHistory(filterLength, 0.0) {


		DEPRECATED_QATOMIC_OPS

Cleanup and deprecate more util/ classes #13687

Are you sure you want to change the base?

Cleanup and deprecate more util/ classes #13687

Conversation

Swiftb0y commented Sep 23, 2024

acolombier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Sep 23, 2024

daschuer Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cleanup and deprecate more `util/` classes #13687

Cleanup and deprecate more `util/` classes #13687

Swiftb0y Sep 23, 2024 •

edited

Loading

daschuer Sep 24, 2024 •

edited

Loading