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

CXX-Qt-lib: Implement QHash<K,V> instead of QHash<QHashPair_K_V> #1138

Open
LeonMatthesKDAB opened this issue Dec 3, 2024 · 0 comments
Open
Labels
👷 refactor Something needs to change 🥳🎉 1.0 This issue is part of stabilization for 1.0 release

Comments

@LeonMatthesKDAB
Copy link
Collaborator

Idea

It turns out there is an exception to the orphan rule that we can exploit to make QHash<K,V> work that we didn't think of before.

Basically, whilst you generally cannot implement a foreign trait for a foreign type, you can actually do so, if the trait is templated and one of the template arguments is a local type!

So if we make the QHashPair trait templated over the Key and the Value, we can actually make this work:

pub struct QHash<Key, Value>
where
    QHash<Key, Value>: QHashPair<Key, Value>,
{
    _space: MaybeUninit<usize>,
    _key: PhantomData<Key>,
    _value: PhantomData<Value>,
}

It would then be possible for users to:

impl cxx_qt_lib::QHashPair<cxx_qt_lib::QString, MyType>
    for cxx_qt_lib::QHash<cxx_qt_lib::QString, MyType> {
// ....
}

because MyType is a local type, this is possible, even if the target type and trait are coming from cxx_qt_lib.

Backwards compatibility

The most problematic part of this refactoring is backwards-compatibility.
Our current tutorial videos already show QHash<QHashPair_K_V> and we want to keep those tutorials working as much as possible.

We may be able to solve this issue by using default type paramters, e.g. by setting V=(), we would allow users to use a QHashPair_K_V as the Key whilst leaving the value blank.

Because all behavior in the end is delegated to the trait anyway, we can implement QHashPair<QHashPair_K_V, ()>, while also adding a deprecation notice to the QHashPair_K_V.

Note that we likely want to remove this entirely for the final 1.0 release, as this solution is likely to require some contortion on the implementation of QHash.

@LeonMatthesKDAB LeonMatthesKDAB added 🥳🎉 1.0 This issue is part of stabilization for 1.0 release 👷 refactor Something needs to change labels Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👷 refactor Something needs to change 🥳🎉 1.0 This issue is part of stabilization for 1.0 release
Projects
None yet
Development

No branches or pull requests

1 participant