Skip to content

Conversation

isaevil
Copy link
Contributor

@isaevil isaevil commented Sep 8, 2025

Description

Use setjmp/std::longjmp for custom assertion handler in testing instead of exceptions for test builds with -fno-exception flag.

Fixes # - issue number(s) if exists

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

@isaevil isaevil marked this pull request as ready for review September 8, 2025 15:23
aleksei-fedotov
aleksei-fedotov previously approved these changes Sep 8, 2025
Copy link
Contributor

@aleksei-fedotov aleksei-fedotov left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines 81 to 87
bool okay = false; \
if (setjmp(utils::g_assertion_jmp_buf)) { \
okay = true; \
message = utils::g_assertion_failure->message; \
} else { \
x; \
} \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to swap if and else branches to better match the original test?
I also thought about keeping only this implementation as the only test:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Going one step further, I had an idea to merge two implementations as several lines are identical. However, it might negatively impact the overall readability. Although the macro will be smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the exception implementation is better overall and should be used when possible.

I had an idea to merge two implementations as several lines are identical. However, it might negatively impact the overall readability. Although the macro will be smaller.

This macro is relatively small, so small code duplication is OK here.

I swapped branches.

kboyarinov
kboyarinov previously approved these changes Sep 9, 2025
@isaevil isaevil merged commit b1d5aa2 into master Sep 9, 2025
29 checks passed
@isaevil isaevil deleted the dev/isaevil/assertion_handler_noexcept branch September 9, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants