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 new_ptr template #905

Merged
merged 4 commits into from
Apr 4, 2024
Merged

add new_ptr template #905

merged 4 commits into from
Apr 4, 2024

Conversation

knoxfighter
Copy link
Contributor

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

see #823

@knoxfighter
Copy link
Contributor Author

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
Copy link
Collaborator

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 🥳

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? 😅 🙈

@knoxfighter
Copy link
Contributor Author

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.

Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

@knoxfighter Thank you a lot for this contribution. Happy to see we have this finally done with perfect forwarding 👍

@LeonMatthesKDAB LeonMatthesKDAB enabled auto-merge (squash) April 3, 2024 19:14
@LeonMatthesKDAB
Copy link
Collaborator

@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.

auto-merge was automatically disabled April 3, 2024 20:11

Head branch was pushed to by a user without write access

@knoxfighter
Copy link
Contributor Author

knoxfighter commented Apr 3, 2024

@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.

@ahayzen-kdab ahayzen-kdab enabled auto-merge (rebase) April 4, 2024 07:51
@ahayzen-kdab ahayzen-kdab merged commit c2320fc into KDAB:main Apr 4, 2024
10 checks passed
@knoxfighter knoxfighter deleted the new-ptr branch April 4, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants