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

Add migration code for QMutex/QRecursiveMutex/QMutexLocker #4096

Merged
merged 1 commit into from
Jul 10, 2021
Merged

Add migration code for QMutex/QRecursiveMutex/QMutexLocker #4096

merged 1 commit into from
Jul 10, 2021

Conversation

uklotzde
Copy link
Contributor

A quick PoC for portable QMutex/QRecursiveMutex/QMutexLocker code.

Not tested with Qt6 yet.

@uklotzde uklotzde requested a review from Be-ing July 10, 2021 11:59
@coveralls
Copy link

coveralls commented Jul 10, 2021

Pull Request Test Coverage Report for Build 1018058343

  • 87 of 147 (59.18%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.0008%) to 28.627%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/track/track.cpp 85 145 58.62%
Files with Coverage Reduction New Missed Lines %
src/engine/readaheadmanager.cpp 2 88.79%
Totals Coverage Status
Change from base Build 1017932377: -0.0008%
Covered Lines: 20082
Relevant Lines: 70150

💛 - Coveralls

@uklotzde
Copy link
Contributor Author

I have chosen to migrate Track because it probably covers all use cases.

@uklotzde
Copy link
Contributor Author

This is also an example why using auto instead of literally repeating the actual type declaration everywhere is useful.

@daschuer
Copy link
Member

daschuer commented Jul 10, 2021

This is also an example why using auto instead of literally repeating the actual type declarations everywhere is useful.

Right, this overload and also template use case is the perfect fit for auto. I it also still fits to our rule to use auto to avoid repeatiton.

I don't think that it rectifies the switch to always use outo. Apart from the readability issues of diffs and code outside the editor it also makes the recent type refactoring less effective because some type checks are bypassed when using auto.

@daschuer
Copy link
Member

LGTM. Thank you for wrapping you head around this hard nut.

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

Waiting for CI

@daschuer
Copy link
Member

This is still draft state. Do we need some manual tests?

@uklotzde
Copy link
Contributor Author

The compiler is the test.

The PR is marked as Draft to ask for feedback. Maybe someone has other/better ideas? It was just a quick test. I don't care how it finally looks like. It should just work.

@Be-ing Be-ing marked this pull request as ready for review July 10, 2021 15:02
@Be-ing Be-ing merged commit 7cad4cc into mixxxdj:main Jul 10, 2021
@uklotzde
Copy link
Contributor Author

This is also an example why using auto instead of literally repeating the actual type declarations everywhere is useful.

Right, this overload and also template use case is the perfect fit for auto. I it also still fits to our rule to use auto to avoid repeatiton.

I don't think that it rectifies the switch to always use outo. Apart from the readability issues of diffs and code outside the editor it also makes the recent type refactoring less effective because some type checks are bypassed when using auto.

If using auto introduces any weakness you should first and foremost rethink your type system, i.e. by using strict types and preventing implicit conversions. Implicit conversions between various native types are a weakness of C/C++.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2021

You may want to share this on the Qt forum. I am sure others will find it useful.

@uklotzde
Copy link
Contributor Author

I am not particularly proud of this simplistic solution 😅

Moreover we haven't tested it with Qt6 yet. It's an ugly workaround and Qt should have provided a better migration path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants