Skip to content
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

Allow throwing from exit handler #349

Merged
merged 1 commit into from
May 9, 2020

Conversation

ripopov
Copy link
Contributor

@ripopov ripopov commented May 8, 2020

For example for unit testing purposes we may want to replace SIGABRT with
throwing an exception, so that unit tests for CHECK()s can be written.

Solves #343

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

Throwing from a destructor is a big no-no in C++ which I didn’t think of immediately. I’ve to think on this one.

Regardless, please add unit test for this that has EXPECT_THROW
Just a pet peeve of mine. Your description isn’t accurate!
Writing unit tests with CHECK is possible without this. It’s just not possible at the moment to do it exactly the way you want to do it.

The way it was approached before is a safer way to do it than mucking with throwing destructors. This is clear in the issue you raised and can be seen throughout test_io.cpp

For example for unit testing purposes we may want to replace SIGABRT with
throwing an exception, so that unit tests for CHECK()s can be written.
@ripopov ripopov force-pushed the THROW_FROM_CHECK branch from 48650a9 to ab3a5f7 Compare May 9, 2020 00:03
@ripopov
Copy link
Contributor Author

ripopov commented May 9, 2020

Added unit test.

Throwing from a destructor is a big no-no in C++ which I didn’t think of immediately. I’ve to think on this one.

In general yes. However, this usage case, where we utilize destructor of temporary object to do logging, could be a legitimate exception of this rule.

@ripopov
Copy link
Contributor Author

ripopov commented May 9, 2020

The way it was approached before is a safer way to do it than mucking with throwing destructors.

Don't agree. If invariant is broken, we should not continue. Like in unit test I've added, we should not allow code to continue and deference an invalid pointer.

Copy link
Owner

@KjellKod KjellKod left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution. It's cool to see different ways and approaches

@KjellKod KjellKod merged commit 751330b into KjellKod:master May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants