KDAB / cxx-qt

Safe interop between Rust and Qt
https://kdab.github.io/cxx-qt/book/
973 stars 67 forks source link

add new_ptr template #905

Closed knoxfighter closed 3 months ago

knoxfighter commented 3 months ago

This PR also includes small changes to make_unique and make_shared, so they are properly forwarding their arguments.

see #823

knoxfighter commented 3 months ago

forwarding does not seem to work when used with some parameters.

error C2440: 'initializing': cannot convert from 'T *(__cdecl *)(Args &&...)' to 'QLabel *(__cdecl *)(const QString &,QWidget *)'
note: None of the functions with this name in scope match the target type
::QLabel *cxxbridge1$QLabel_new_builder(::QString const &text, ::QWidget *parent) noexcept {
  ::QLabel *(*QLabel_new_builder$)(::QString const &, ::QWidget *) = ::new_ptr;
  // ^ line with the error
  return QLabel_new_builder$(text, parent);
}

I was able to reproduce it: https://godbolt.org/z/baoYbaE7v Can you explain me why that issue exists and how to get rid of it? Possible it is a non-issue to just copy all params as it was before 🤔

LeonMatthesKDAB commented 3 months ago

forwarding does not seem to work when used with some parameters.

error C2440: 'initializing': cannot convert from 'T *(__cdecl *)(Args &&...)' to 'QLabel *(__cdecl *)(const QString &,QWidget *)'
note: None of the functions with this name in scope match the target type
::QLabel *cxxbridge1$QLabel_new_builder(::QString const &text, ::QWidget *parent) noexcept {
  ::QLabel *(*QLabel_new_builder$)(::QString const &, ::QWidget *) = ::new_ptr;
  // ^ line with the error
  return QLabel_new_builder$(text, parent);
}

I was able to reproduce it: https://godbolt.org/z/baoYbaE7v Can you explain me why that issue exists and how to get rid of it? Possible it is a non-issue to just copy all params as it was before 🤔

Ah, that error. I've encountered something similar when first playing around with make_unique and friends.

That's why the current functions aren't using universal references, I hadn't tried it extensively and just used what worked

I've now played around with this a bit more. And I believe we don't actually need universal references in this case, but we still want to use std::forward<Args>(args)....

TL;DR

Replace Args&&... args with Args... args, but keep the std::forward<Args>(args)... in all functions. Then we should have perfect forwarding!

Long explanation

See this code: https://godbolt.org/z/nKKd6Wh3q Which when I run it correctly emits:

const int &, int
int &, int
int &&, int

The reasoning is as follows:

C++ needs "universal references" because of it's template argument deduction rules. The issue is that a template like this:

template<typename T>
void foo(T t) {
    // ...
}

when called with foo(5), resolves to foo<int>, not foo<int&> (see the cpp reference). As explained here, you can solve this by declaring the function as: void foo(T &t). However, then you could only use l-value references, and no longer r-value references.

That's why C++ introduces a special handling of r-value references in template deduction. That essentialy says T &&t will result in an l-value reference when called with an l-value. This is what universal references are!

However, we don't need this! CXX explicitly casts the function to a specific type, it already enforces the correct template deduction, including the reference!

::QLabel *(*QLabel_new_builder$)(::QString const &, ::QWidget *) = ::new_ptr;

This forces Args... to be essentially ::QString const &, ::QWidget *, as now it can not favor deduction of ::QString over ::QString const &. So this already results in the correct l-value reference, as needed for forwarding :partying_face:

We then still need std::forward<Args>(args)..., to make the forwarding perfect, as it will otherwise turn r-value references into l-value references.

Using Args&& doesn't work, as now the function must take an r-value reference. In our case it would be an r-value reference to an l-value reference (so a && &, essentially), which would collapse to an l-value reference again, so it doesn't matter if you call this function. But it seems our C++ compiler can't do this implicitly when checking the function pointer type.

Damn, C++ is fun, isn't it? :sweat_smile: :see_no_evil:

knoxfighter commented 3 months ago

Thank you very much for that deep explanation. Yes, C++ is fun and sometimes cursed. I removed the universal reference and fixed the missing semicolon.

LeonMatthesKDAB commented 3 months ago

@knoxfighter please rebase this branch onto the main branch, then it should merge automatically. You may be able to do this via the Github UI directly.

Unfortunately I can't do it for you, as I don't have write access to your repo.

knoxfighter commented 3 months ago

@knoxfighter please rebase this branch onto the main branch, then it should merge automatically. You may be able to do this via the Github UI directly.

Unfortunately I can't do it for you, as I don't have write access to your repo.

the automerge was cancelled by the force-push github did as i rebased the PR onto main.