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

Import CGFloat as CoreFoundation.CGFloat if possible #40856

Merged
merged 5 commits into from Jan 21, 2022

Conversation

salinas-miguel
Copy link
Contributor

Resolves rdar://87605416

@@ -250,6 +250,29 @@ getSwiftStdlibType(const clang::TypedefNameDecl *D,
}
#include "MappedTypes.def"

if (Name.str() == "CGFloat") {
SwiftModuleName = StringRef("CoreGraphics");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the StringRef()s are necessary

@CodaFi
Copy link
Member

CodaFi commented Jan 16, 2022

@swift-ci test source compatibility

@salinas-miguel
Copy link
Contributor Author

Multiple source failures producing this error: error: emit-module command failed due to signal 6 (use -v to see invocation)Assertion failed: (CMU && CMU->isTopLevel() && "Only top-level modules supported"), function FilteringDeclaredDeclConsumer, file ClangImporter.cpp, line 2767.
I don't understand what's failing here. Is there any way to to see which source file fails to compile now?

@salinas-miguel
Copy link
Contributor Author

Also, is there a way to run this test suite locally?

@nkcsgexi nkcsgexi requested a review from xymus January 18, 2022 19:28
@nkcsgexi
Copy link
Member

Also adding @xymus as an additional reviewer. Do we really need to hard-code this information into the importer? Does it imply a deeper issue in modulization.

@beccadax
Copy link
Contributor

@swift-ci please smoke test

@beccadax
Copy link
Contributor

@swift-ci please test source compatibility

@airspeedswift
Copy link
Member

@swift-ci please smoke test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This looks okay if it's passing tests

M = Impl.getNamedModule("CoreGraphics");
SwiftType = Impl.getNamedSwiftType(M, SwiftTypeName);
}

if (!SwiftType && !CanBeMissing) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a slight behavior change here that might explain the failure in module-cache-diagnostics.swift: when there is no module and CanBeMissing, the old code would unconditionally return std::make_pair(Type(), ""). The new code will return std::make_pair(Type(), SwiftTypeName).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I'll add the module check back

@salinas-miguel
Copy link
Contributor Author

The lldb failure was due to the fact that lldb reuses the ClangImporter/MappedTypes.def file. Inverting the fallback logic and keeping the MappedTypes.def file unmodified fixes the failure. This should pass the smoke test now. Can someone please run the smoke test?

@airspeedswift
Copy link
Member

@swift-ci please smoke test

@salinas-miguel
Copy link
Contributor Author

The Windows failure looks unrelated. #40954 is also hitting the same failure.

@salinas-miguel
Copy link
Contributor Author

@swift-ci please test Windows

@salinas-miguel salinas-miguel merged commit c356875 into apple:main Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants