[cxx-interop] Generate safe overloads for non-escapable spans #78422
Conversation
Xazax-hun
requested review from
zoecarver,
hyp,
egorzhdan,
a team,
beccadax and
ian-twilightcoder
as code owners
| func getUnqualifiedStdName(_ type: String) -> String? { | ||
| if (type.hasPrefix("std.")) { | ||
| var ty = type.dropFirst(4) | ||
| if (ty.hasPrefix("__1.")) { |
There was a problem hiding this comment.
Will it always be __1., or could it also be __2.?
There was a problem hiding this comment.
I do not anticipate a change in this, it is the same namespace for the last decade or so. But it is true that we are somewhat closely coupled to this implementation detail.
There was a problem hiding this comment.
Should we make this adjustment on the C++ side where we can reason about things like inline namespaces?
There was a problem hiding this comment.
I think it would be reasonable to skip inline namespaces in the Swift names, especially since these names do leak into the diagnostics occasionally. It would be more user friendly. I am not sure if this could ever create name collisions, hopefully not. But I'd address this in a separate PR.
How closely coupled are the compiler and the standard library? In case this code supposed to work with older compiler versions, we'd want to keep the skipping logic here.
Xazax-hun
force-pushed
the
gaborh/cxx-span-overload-importer
branch
from
6570674 to
25facce
Compare
| @@ -580,7 +580,8 @@ void importer::getNormalInvocationArguments( | |||
| } | |||
| } | |||
|
|
|||
| if (LangOpts.hasFeature(Feature::SafeInteropWrappers)) | |||
| if (LangOpts.hasFeature(Feature::SafeInteropWrappers) && | |||
There was a problem hiding this comment.
I saw some crashes in Clang when this option was used in C++ mode.
There was a problem hiding this comment.
That's fine for now, but we should dig into these crashes to see what's going on.
There was a problem hiding this comment.
-fbounds-safety support for C++ in Clang is currently experimental, but -fexperimental-bounds-safety-attributes is used for C/C++ interop also, so that one shouldn't crash. It is a relatively new flag though, so there might be some gaps. CC @patrykstefanski for awareness.
Xazax-hun
force-pushed
the
gaborh/cxx-span-overload-importer
branch
2 times, most recently
from
a09d1f0 to
487e154
Compare
| @@ -580,7 +580,8 @@ void importer::getNormalInvocationArguments( | |||
| } | |||
| } | |||
|
|
|||
| if (LangOpts.hasFeature(Feature::SafeInteropWrappers)) | |||
| if (LangOpts.hasFeature(Feature::SafeInteropWrappers) && | |||
There was a problem hiding this comment.
That's fine for now, but we should dig into these crashes to see what's going on.
| func getUnqualifiedStdName(_ type: String) -> String? { | ||
| if (type.hasPrefix("std.")) { | ||
| var ty = type.dropFirst(4) | ||
| if (ty.hasPrefix("__1.")) { |
There was a problem hiding this comment.
Should we make this adjustment on the C++ side where we can reason about things like inline namespaces?
A previous PR already added support to the SwiftifyImport macro to generate safe wrappers. This PR makes ClangImporter emit the macro to do the transformation.
Xazax-hun
force-pushed
the
gaborh/cxx-span-overload-importer
branch
from
487e154 to
4846c56
Compare
|
@swift-ci please smoke test |
A previous PR already added support to the SwiftifyImport macro to generate safe wrappers. This PR makes ClangImporter emit the macro to do the transformation.