Skip to content

Swift: fix extractor built with NDEBUG #9264

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

Merged
merged 1 commit into from
May 23, 2022

Conversation

redsun82
Copy link
Contributor

There was a call with side effects in an assert, that was therefore
not being called with NDEBUG turned on, changing extractor results.

@redsun82 redsun82 requested a review from AlexDenisov May 23, 2022 10:33
@redsun82 redsun82 requested a review from a team as a code owner May 23, 2022 10:33
@github-actions github-actions bot added the Swift label May 23, 2022
AlexDenisov
AlexDenisov previously approved these changes May 23, 2022
@@ -153,7 +153,9 @@ class SwiftDispatcher {
template <typename Tag, typename... Ts>
TrapLabel<Tag> fetchLabelFromUnion(const llvm::PointerUnion<Ts...> u) {
TrapLabel<Tag> ret{};
assert((... || fetchLabelFromUnionCase<Tag, Ts>(u, ret)) && "llvm::PointerUnion not set");
// with logical op short-circuiting, this will stop trying on the first successful fetch
bool unionCaseFound = (... || fetchLabelFromUnionCase<Tag, Ts>(u, ret));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be marking these with __used or similar, otherwise there are lots of "unused variable" warnings when built without assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I saw a lot of that as well, however maybe I would not do that before we roll out the shiny Diagnostic module that will save us all, as those warnings will probably go away anyway with that... We could maybe wrap asserts into a function maybe to avoid the warnings for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine as it is right now, we can just wait for the diagnostics

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the motivating use cases for [[maybe_unused]] (and indeed the example at https://en.cppreference.com/w/cpp/language/attributes/maybe_unused).

And of course for more complex cases, another option is checking NDEBUG ourselves:

#ifdef NDEBUG
  bool unionCaseFound = 
#endif
      (... || fetchLabelFromUnionCase<Tag, Ts>(u, ret));
  assert(unionCaseFound && "llvm::PointerUnion not set to a known case");

Here, I think [[maybe_unused]] is the right "hammer": it's not very noisy, and does silence the warning (at the risk of leaving the unused variable if the assert goes away - but that's no worse a risk than learning to ignore warnings!). But checking NDEBUG can be useful when we don't want to run something in a release mode, but it doesn't comfortably fit inside an assert condition (e.g. if we'd written this as a loop instead of a fold).

There was a call with side effects in an `assert`, that was therefore
not being called with `NDEBUG` turned on, changing extractor results.
@redsun82 redsun82 force-pushed the redsun82/swift-fix-ndebug-build branch from be8209d to ea6a249 Compare May 23, 2022 10:36
@redsun82 redsun82 merged commit 63f5a86 into main May 23, 2022
@redsun82 redsun82 deleted the redsun82/swift-fix-ndebug-build branch May 23, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants