-
-
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
refactor(controllers): send sysex to all handlers #12827
refactor(controllers): send sysex to all handlers #12827
Conversation
LGTM! Thank you! |
Thank you. |
auto it = m_pMapping->getInputMappings().constFind(mappingKey.key); | ||
for (; it != m_pMapping->getInputMappings().constEnd() && it.key() == mappingKey.key; ++it) { | ||
processInputMapping(it.value(), data, timestamp); | ||
const auto inputMappings = m_pMapping->getInputMappings().values(mappingKey.key); |
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.
values() returns a fresh allocated QList()
https://github.com/qt/qtbase/blob/adc0920c48d812fb1b3e2b7bfc76217a7126a41e/src/corelib/tools/qhash.h#L1088C38-L1088C63
This happens for every message and should be avoided.
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.
Damn, I tought the QMultiHash was using them internally and just returned an implicitly-shared copy. I should've checked.
@Swiftb0y can we find a solution that does not allocate memory?
I didn't at first, but I just found it now, It's https://doc.qt.io/qt-6/qmultihash.html#equal_range. PR underway.
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.
Thank you.
@Swiftb0y can we find a solution that does not allocate memory? |
Something I noticed while digging through #12824, probably a bug, but its masked by the unintuitive behavior described in #12824 anyways. Potentially proper fixes for #12824 to come.