Skip to content

[cxx Interop] Disambiguate methods within the same class with same name but differing "constness" #41308

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
merged 4 commits into from
Feb 11, 2022

Conversation

Huddie
Copy link
Contributor

@Huddie Huddie commented Feb 10, 2022

When importing CXX methods into Swift the compiler had issues with classes (Records) that contained 2+ methods that differed in "constness" but had the same name

Example:

int increment(int a);
int increment(int a) const;

This PR appends "Mutating" to the end of the mutable function to disambiguate when importing into swift.

the result of the above would then be:

func incrementMutating(_ a: Int32) -> Int32
func increment(_ a: Int32) -> Int32

Determining when 2 methods conflict is a seemingly hard problem. Name alone is not enough to get it right 100% of the time since you could have a class with:

int increment(int a);
int increment(int a, int b) const;

That said, such patterns seem less common and so this PR uses name alone to disambiguate, recognizing that it may result in some confusing outputs in more uncommon circumstances

The above example would result in:

func incrementMutating(_ a: Int32) -> Int32
func increment(_ a: Int32, _ b: Int32) -> Int32

even though the 2 methods do NOT conflict.

@Huddie Huddie requested a review from zoecarver February 10, 2022 00:56
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

This PR is fantastic. Thank you, Ehud!

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Feb 10, 2022
@zoecarver zoecarver requested a review from hyp February 10, 2022 01:01
@zoecarver
Copy link
Contributor

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor

@swift-ci please smoke test.


// No input with output
int numberOfMutableMethodsCalled() { return ++mutableMethodsCalledCount; }
int numberOfMutableMethodsCalled() const { return mutableMethodsCalledCount; }
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like your test cases are in a pattern of:

-  mutating method
- const method

Could you also add another method pair in reverse order, i.e.

- const method
- mutating method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

@hyp
Copy link
Contributor

hyp commented Feb 10, 2022

LG with one suggestion.

However, I believe we need a discussion if adding 'Mutating' at the end of the method is the right approach with respect to the naming convention. A 'mutating...' prefix might be ergonomic. Could you write up a short pitch for this on the Swift forums where we can have a discussion about the naming?

@zoecarver
Copy link
Contributor

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor

@swift-ci please smoke test macOS.

@zoecarver zoecarver merged commit 5e1e0c1 into swiftlang:main Feb 11, 2022
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.

3 participants