Skip to content

Conversation

@egorzhdan
Copy link
Contributor

When converting a C++ class template instantiation name into Swift, we previously didn't account for possible SIMD types. Those types were printed as _. This meant that e.g. std::vector<simd::float3> and std::vector<simd::float4> would get the same Swift name, causing compiler errors down the road.

rdar://134214091

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Sep 4, 2024
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is probably not performance critical code, but usually it is more efficient to use llvm::Twine to concat strings, like:
(Twine("foo") + "bar" + "baz").str();

Feel free to leave as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, switched this to use Twine.

@egorzhdan egorzhdan force-pushed the egorzhdan/simd-template-param branch from 2d29a2a to 484a5af Compare September 4, 2024 14:05
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

Choose a reason for hiding this comment

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

I think with twine we might no longer need the to_string. But not 100% confident on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't compile for me if I remove to_string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe it still needs a ctor like Twine("SIMD") + Twine(type->getNumElements()) + ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think I slightly prefer to_string over Twine(...) in this case, since that makes it explicit that we aren't e.g. constructing a string with a given capacity/size – something a std::string constructor would do 🙂

When converting a C++ class template instantiation name into Swift, we previously didn't account for possible SIMD types. Those types were printed as `_`. This meant that e.g. `std::vector<simd::float3>` and `std::vector<simd::float4>` would get the same Swift name, causing compiler errors down the road.

rdar://134214091
@egorzhdan egorzhdan force-pushed the egorzhdan/simd-template-param branch from 484a5af to d19c9eb Compare September 4, 2024 15:46
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan merged commit 9ac56f2 into main Sep 5, 2024
@egorzhdan egorzhdan deleted the egorzhdan/simd-template-param branch September 5, 2024 11:46
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