[interop][SwiftToCxx] Gather initial struct layout information and em… #59119
Conversation
|
@swift-ci please test |
| @@ -25,9 +24,22 @@ | |||
| using namespace swift; | |||
|
|
|||
| void ClangValueTypePrinter::printStructDecl(const StructDecl *SD) { | |||
| auto structABIInfo = interopContext.getStructDeclInfo(SD); | |||
| if (!structABIInfo) { | |||
| // FIXME: handle non-fixed layout structs. | |||
There was a problem hiding this comment.
Does it make sense to print out something here like <resilient-struct>?
There was a problem hiding this comment.
Maybe print out an unavailable forward class declaration?
| return; | ||
| } | ||
| if (structABIInfo->size == 0) { | ||
| // FIXME: How to represent 0 sized structs? |
There was a problem hiding this comment.
Should this be an assertion?
There was a problem hiding this comment.
A user-facing error might be better. Or alternatively we can emit a class that's marked as unavailable, wdyt?
| @@ -1,20 +1,26 @@ | |||
| // RUN: %empty-directory(%t) | |||
| // RUN: %target-swift-frontend %s -typecheck -module-name Structs -clang-header-expose-public-decls -emit-clang-header-path %t/structs.h | |||
| // RUN: %target-swift-frontend %s -typecheck -emit-ir -module-name Structs -clang-header-expose-public-decls -emit-clang-header-path %t/structs.h | |||
There was a problem hiding this comment.
Why does this need the IR now?
There was a problem hiding this comment.
Right now I'm getting the layout info after canonical SIL is constructed. I definitely need SIL to gather the layout info. That's what I mentioned in my description:
Note: the current header emission logic does not yet force SILGen/IRGen, so we will only emit types when the compiler gets to IRGen. This is something that will be addressed in the future (we should probably require SILGen/IRGen for header emission?)
I think that's something I should ask John in the sync-up as well.
| @@ -0,0 +1,61 @@ | |||
| //===--- SwiftIRDetailsCollector.h - Get ABI details for decls --*- C++ -*-===// | |||
There was a problem hiding this comment.
I wonder if we could just expose the type info from IRGen? It would be a big-ish refactor, but maybe we could just factor that API out of IRGen? Thoughts?
There was a problem hiding this comment.
I was thinking about that, but I would need to pull out a lot of logic out of IRGen right now, and it's unclear how much I actually need just to get the layout and ABI right. Perhaps it's the right thing to do, but it's not obvious to me just yet. This is something I can discuss with @rjmccall in our sync up. For now I can actually refactor this to pull out specific query interface instead of having a visitor .
There was a problem hiding this comment.
We decided to provide a high level interface for now that's pretty much decoupled from IRGen / SILGen but implemented using IRGen by setting up the required context, so that we can avoid depending on running SILGen/IRGen in order to emit the header.
There was a problem hiding this comment.
I will update the patch to reflect that.
|
@swift-ci please test linux platform |
|
@swift-ci please test |
…it struct stubs with storage in C++ This change extends the clang header printer to start emitting C++ classes for Swift struct types with the correct struct layout in them (size + alignment)
hyp
force-pushed
the
eng/swift-struct-layout-in-cxx
branch
from
b89c0ea to
42c6ff6
Compare
|
Updated. |
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
3 similar comments
|
@swift-ci please test source compatibility |
|
@swift-ci please test source compatibility |
|
@swift-ci please test source compatibility |
…it struct stubs with storage in C++
This change extends the clang header printer to start emitting C++ classes for Swift struct types with the correct struct layout in them (size + alignment)
Note: the current header emission logic does not yet force SILGen/IRGen, so we will only emit types when the compiler gets to IRGen. This is something that will be addressed in the future (we should probably require SILGen/IRGen for header emission?)