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 extern C markers #204

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Conversation

brianmichel
Copy link
Contributor

@brianmichel brianmichel commented Oct 17, 2023

Ensure C linkage for shims to allow for usage in C++ interop environments.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

This library when consumed from a swift package that has C++ interop enabled fails to successfully link since the symbols seem to get mangled.

Modifications:

I've added conditional extern "C" markers in the shims file if and only iff C++ is detected.

Result:

You should be able to consume and link this library in a target that has C++ interop enabled.

Copy link
Contributor

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I feel uneasy about this. This only impacts the implementation and not the declaration which are consumed by the ClangImporter and thus this can lead to a mismatch in the linkage.

@brianmichel
Copy link
Contributor Author

I feel uneasy about this. This only impacts the implementation and not the declaration which are consumed by the ClangImporter and thus this can lead to a mismatch in the linkage.

Ah this should likely just be in the header files, right? Sorry, long day of debugging.

Copy link
Contributor

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

@swift-server-bot please test

@Lukasa
Copy link
Collaborator

Lukasa commented Oct 18, 2023

@swift-server-bot test this please

@Lukasa Lukasa added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Oct 18, 2023
@compnerd
Copy link
Contributor

@Lukasa can we get this merged please?

@Lukasa Lukasa merged commit fade397 into apple:main Oct 20, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants