Skip to content

[cxx-interop] Support reference types in function templates. #34536

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

Merged

Conversation

zoecarver
Copy link
Contributor

Support reference wrapped template params such as:

template <class T> void lvalueReference(T &ref)

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Nov 1, 2020
Copy link
Contributor

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Thanks Zoe, LGTM!

if (auto *templateParamType =
dyn_cast<clang::TemplateTypeParmType>(paramTy)) {

if (isa<clang::ReferenceType>(paramTy) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use if (auto referenceType = dyn_cast<...>? for both referenceType and for templateParamType? I think it will make the body more readable at the price of increased indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue is then the else if doesn't work. I could introduce a variable isReferenceWrappedTempalte but that basically has the same issue as the current if statement. Happy to do that if you'd rather, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced the calls to isa<clang::ReferenceType> and cast<clang::ReferenceType> with a dyn_cast<clang::ReferenceType> at the top of the if. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, yeah then it cannot be super clean anyway. The code we how now LGTM. Thanks!

@zoecarver zoecarver force-pushed the cxx/fix/reference-wrapped-templates branch from 8cd8347 to c6aef37 Compare November 6, 2020 06:03
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver merged commit cb7bb52 into swiftlang:main Nov 7, 2020
ainu-bot added a commit to google/swift that referenced this pull request Nov 7, 2020
* 'main' of github.com:apple/swift:
  [cxx-interop] Don't crash for unkown inline-operator. (swiftlang#34602)
  [cxx-interop] Support reference types in function templates. (swiftlang#34536)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants