-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Swift: skip declarations marked as unavailable #14437
Conversation
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. |
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.