Fixes for __attribute__((ns_consumed)) block parameters #28527
Conversation
…eters Fixes <rdar://problem/48792177>.
|
Ugh... |
|
@swift-ci Please smoke test |
|
@swift-ci Please clean smoke test |
| @@ -594,7 +597,9 @@ clang::CanQualType GenClangType::visitSILFunctionType(CanSILFunctionType type) { | |||
| break; | |||
|
|
|||
| case ParameterConvention::Direct_Owned: | |||
| llvm_unreachable("block takes owned parameter"); | |||
| extParamInfo = extParamInfo.withIsConsumed(true); | |||
| break; | |||
There was a problem hiding this comment.
@varungandhi-apple You'll need to make an analogous change.
There was a problem hiding this comment.
(Comment for my own reference): #27479 needs to have an analogous change in ClangTypeConverter.cpp.
There was a problem hiding this comment.
Actually, we don't need to make any changes since visitSILFunctionType in ClangTypeConverter is unreachable -- it should receive only AST types.
There was a problem hiding this comment.
owned can appear in AST function parameters as well; it's in the ParameterTypeFlags.
| "# define SWIFT_RELEASES_ARGUMENT __attribute__((ns_consumed))\n" | ||
| "#else\n" | ||
| "# define SWIFT_RELEASES_ARGUMENT\n" | ||
| "#endif\n" |
There was a problem hiding this comment.
I take it it's general policy to use Swift-specific macros here (which we can just add whenever we need them) rather than the NS ones?
| clang::FunctionProtoType::ExtProtoInfo defaultEPI; | ||
| auto fnTy = clangCtx.getFunctionType(resultType, paramTypes, defaultEPI); | ||
| clang::FunctionProtoType::ExtProtoInfo extProtoInfo; | ||
| extProtoInfo.ExtParameterInfos = extParamInfos.begin(); |
There was a problem hiding this comment.
@slavapestov -- is SmallVector guaranteed to have contiguous storage in the resizing case? (If not, then this wouldn't work because there's no way to recover the split storage since ExtParameterInfos is a pointer). And is clangCtx.getFunctionType guaranteed to create a copy of this array? Otherwise, there's a lifetime problem.
There was a problem hiding this comment.
Do you know if these points are documented anywhere? I looked at the Doxygen and I couldn't find this. Also, is there some general guidelines at play here (e.g. "getFooType" always deep-copies everything in Clang) or is this a "yes, I know these particular points are true"?
There was a problem hiding this comment.
All AST nodes are supposed to be self-contained in both Clang and Swift (except inasmuch as they reference other AST nodes, which should themselves be self-contained). Creating an AST node should always be deep-copying any necessary arrays into trailing storage or some other associated storage.
There might be exceptions for TypeVariableTypes in Swift, they're a weird case.
Fixes rdar://problem/48792177.