8000 Throw std::runtime_exception instead of char*. by anntzer · Pull Request #9289 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Throw std::runtime_exception instead of char*. #9289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 6, 2017
Merged

Throw std::runtime_exception instead of char*. #9289

merged 1 commit into from
Oct 6, 2017

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Oct 5, 2017

This should make it easier to debug exceptions falling out of the C++
code, as the C++ runtime will print the error message (the what() of
the exception) on its way to aborting the program.

Did a regex replace; removed the char* handler in CALL_CPP_FULL; and
remove a bunch of extra whitespace.

For example, before application of the patch in #9074, the failing code now gives

terminate called after throwing an instance of 'std::runtime_error'
  what():  Couldn't close file
Fatal Python error: Aborted

Should also help with #9244.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
Throw std::runtime_exception instead of char*.
df79954 
This should make it easier to debug exceptions falling out of the C++
code, as the C++ runtime will print the error message (the `what()` of
the exception) on its way to aborting the program.

Did a regex replace; removed the `char*` handler in CALL_CPP_FULL; and
remove a bunch of extra whitespace.
@tacaswell
Copy link
Member

Would prefer @mdboom or @ianthomas23 sign off on this before merging.

ianthomas23
ianthomas23 approved these changes Oct 6, 2017
View reviewed changes
Copy link
Member
@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

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

I'm happy with this.

Don't understand the AppVeyor failure.

@tacaswell tacaswell added this to the 2.1.1 (next bug fix release) milestone Oct 6, 2017
@tacaswell
Copy link
Member

We don't either yet, see #9176 and #9282

@tacaswell tacaswell merged commit 89183ba into matplotlib:master Oct 6, 2017
lumberbot-app bot pushed a commit that referenced this pull request Oct 6, 2017
@tacaswell
Backport PR #9289: Throw std::runtime_exception instead of char*.
dc3a82a
@lumberbot-app lumberbot-app bot mentioned this pull request Oct 6, 2017
Merged
@anntzer anntzer deleted the dont-throw-cpp-strings branch October 6, 2017 20:34
@Kojoley
Copy link
Member
Kojoley commented Oct 6, 2017

Hm. CALL_CPP_FULL is used to catch C++ exceptions and convert them to Python. std::runtime_error is not handled by it (you just simply removed char const *e), so all these exception now will be caught by catch (...) and raise in Python with "Unknown exception in %s" message.
Am I wrong?

@anntzer
Copy link
Contributor Author
anntzer commented Oct 6, 2017

Oops. New PR coming.

@anntzer anntzer mentioned this pull request Oct 6, 2017
Merged
6 tasks
QuLogic added a commit that referenced this pull request Oct 6, 2017
@QuLogic
Merge pull request #9297 from matplotlib/auto-backport-of-pr-9289
01871e4 
Backport PR #9289 on branch v2.1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianthomas23 ianthomas23 ianthomas23 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @tacaswell @Kojoley @ianthomas23
0