-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Swift: skip declarations marked as unavailable #14437
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
Conversation
geoffw0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM.
You mentioned an upgrade script - but the .dbscheme hasn't changed - what does this need to do exactly?
I definitely want to see a DCA run before this is merged, to confirm no unintended consequences.
DCA is in progress. dbscheme change is in the other PR which is not strictly related to these changes. |
redsun82
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a sad note about how it seems we can't use full C++20 just yet 🙄
| for (auto member : decl.getMembers()) { | ||
| if (swift::AvailableAttr::isUnavailable(member)) { | ||
| continue; | ||
| } | ||
| entry.members.emplace_back(dispatcher.fetchLabel(member)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to suggest using C++20's views like this
| for (auto member : decl.getMembers()) { | |
| if (swift::AvailableAttr::isUnavailable(member)) { | |
| continue; | |
| } | |
| entry.members.emplace_back(dispatcher.fetchLabel(member)); | |
| } | |
| entry.members = | |
| dispatcher.fetchRepeatedLabels(decl.getAllMembers() | |
| | std::views::filter(std::not_fn(swift::AvailableAttr::isUnavailable))); |
until I tried it out and stumbled upon llvm/llvm-project#52696
The code above requires at least clang-16 to compile 😭
I guess this might be an input to the self contained toolchain, that we should upgrade clang in the process (including on Linux)
Technically, this is a no-op for our test suite, though I'm not sure why the order has changed.
This is required for #14261 as it fixes a crash when we try to extract certain unavailable declarations.