FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Throw std::runtime_exception instead of char*. by anntzer · Pull Request #9289 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:dont-throw-cpp-strings
Oct 6, 2017
ConversationCommits (1)ChecksFiles changed
Merged

Throw std::runtime_exception instead of char*.#9289
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:dont-throw-cpp-strings

Conversation

anntzer commented Oct 5, 2017

Copy link
Copy Markdown
Contributor

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

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 commented Oct 6, 2017

Copy link
Copy Markdown
Member

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

ianthomas23 approved these changes Oct 6, 2017
View reviewed changes

ianthomas23 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm happy with this.

Don't understand the AppVeyor failure.

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

tacaswell commented Oct 6, 2017

Copy link
Copy Markdown
Member

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

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

Kojoley commented Oct 6, 2017

Copy link
Copy Markdown
Member

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 commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

Oops. New PR coming.

anntzer mentioned this pull request Oct 6, 2017
Merged
6 tasks
QuLogic added a commit that referenced this pull request Oct 6, 2017
Merge pull request #9297 from matplotlib/auto-backport-of-pr-9289
01871e4 
Backport PR #9289 on branch v2.1.x
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL