-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cleanup and deprecate more util/
classes
#13687
base: main
Are you sure you want to change the base?
Conversation
* convert class to namespace * replace unecessary includes with forward decls * replace `NULL` and `0` with `nullptr`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a great cleanup! Thanks for looking into this.
The CI seems to be failing, can you look at this?
src/skin/legacy/legacyskinparser.cpp
Outdated
pTransformer = ValueTransformer::parseFromXml(transform, *m_pContext); | ||
pTransformer = | ||
ValueTransformer::parseFromXml(transform, *m_pContext) | ||
.get(); // TODO don't leak here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
? Could it be possible to make pTransformer
a smart pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Yeah, I'll do so asap. though I'm afraid I won't have much time for mixxx soon. |
|
||
double transformInverse(double argument) const override { | ||
if (argument > 0.0) { | ||
return m_compareValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that correct inversion of Equal? Maybe we should add a brief comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't bother checking, I just moved this part from the header into the cpp.
actually I just now realized that QQueue is terribly inefficient in terms of memory use, so that'll take a little while to rectify. |
src/util/compatibility/qatomic.h
Outdated
|
||
DEPRECATED_QATOMIC_OPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
Did you consider to fix the code instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the comment and #define
src/util/rotary.h
Outdated
double m_dLastValue; | ||
int m_iCalibrationCount; | ||
private: | ||
QQueue<double> m_filterHistory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I did something similar in a follow up with I haven't pushed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 7578cfa
m_dLastValue(0.0), | ||
m_iCalibrationCount(0) { | ||
for (int i = 0; i < m_iFilterLength; ++i) { | ||
m_pFilter[i] = 0.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. If the loop is pre-filled with zeros then the mean is heavily distorted until those zeros have been flushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
src/util/class.h
Outdated
// 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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, But than write an explicit advice what to do instead of this macro. A short for of your last comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll try my best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
40c2240
to
7578cfa
Compare
Rotary(qsizetype filterLength) | ||
: m_filterHistory(filterLength, 0.0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note, that this does restore the previous behavior. I have not yet experimented with more dynamic filtering.
This should conclude #13222 along with a bunch of drive by fixes I couldn't resist not to do.