[Swiftify] Adjust _SwiftifyImport invocation to align with the signature #81855
Conversation
hnrklssn
requested review from
Xazax-hun,
zoecarver,
egorzhdan,
beccadax,
ian-twilightcoder,
j-hui,
fahadnayyar and
susmonteiro
as code owners
|
@swift-ci please smoke test |
lib/ClangImporter/ImportDecl.cpp
Outdated
| assert(SwiftifyImportDecl); | ||
| if (!SwiftifyImportDecl) | ||
| return false; | ||
| for (auto *Param : *SwiftifyImportDecl->parameterList) |
There was a problem hiding this comment.
Could this be replaced with llvm::find_if or something similar?
There was a problem hiding this comment.
It could be replaced with any_of, but it wouldn't really be any easier to read imo (nor more concise), and is a bit more annoying to debug with the jumping between stack frames
lib/ClangImporter/ImportDecl.cpp
Outdated
| bool hasMacroParameter(StringRef ParamName) { | ||
| ValueDecl *D = getDecl("_SwiftifyImport"); | ||
| auto *SwiftifyImportDecl = dyn_cast_or_null<MacroDecl>(D); | ||
| assert(SwiftifyImportDecl); |
There was a problem hiding this comment.
Could the compiler ever be used with on old enough SDK that does not have this macro? Or could the compiler be used without the SDK at all? If yes, maybe we don't want to assert here.
There was a problem hiding this comment.
If that's the case I think we have bigger issues: the macro will still be attached. This only handles whether to use the new or old signature. But it's a good point. @DougGregor what's our stance on using this with an SDK that doesn't contain the macro at all?
There was a problem hiding this comment.
Personally, I tend to be defensive against using the compiler with older SDKs unless it's a ton of work. Here, if you can just safely return false on a null SwiftifyImportDecl, I'd do that.
There was a problem hiding this comment.
I added an early exit in swiftify instead, if @_SwiftifyImport doesn't exit
of the macro in the currently laoded macro module Stdlib macros are shipped with the SDK, so we need to make sure that the way we invoke the macro matches the version of the macro in the SDK. This patch skips the `spanAvailability` parameter if it isn't present. There's no good way to test this, because we will always pick up the current macro in our llvm-lit test suite, but I've tested it manually. rdar://152278292
hnrklssn
force-pushed
the
swiftify-availability-check-sdk
branch
from
80834ca to
1d1c642
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@Xazax-hun this is finally ready for another round of review :) |
| } | ||
|
|
||
| void printAvailabilityOfType(StringRef Name) { | ||
| ValueDecl *D = getDecl(Name); | ||
| ValueDecl *D = getKnownSingleDecl(SwiftContext, Name); |
There was a problem hiding this comment.
Should we also be defensive here in case e.g., Span is not available in the standard library used?
There was a problem hiding this comment.
In theory probably yes, but on the other hand we only reach this if _SwiftifyImport is present, and if that's present and Span isn't I would imagine something is very wrong
…y-check-sdk [Swiftify] Adjust _SwiftifyImport invocation to align with the signature (cherry picked from commit e9ba8f8)
…check Cherry-pick "Merge pull request #81855 from hnrklssn/swiftify-availability-check-sdk"
of the macro in the currently laoded macro module
Stdlib macros are shipped with the SDK, so we need to make sure that the way we invoke the macro matches the version of the macro in the SDK. This patch skips the
spanAvailabilityparameter if it isn't present. There's no good way to test this, because we will always pick up the current macro in our llvm-lit test suite, but I've tested it manually.rdar://152278292