-
Notifications
You must be signed in to change notification settings - Fork 50
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
Preserve exception #130
Preserve exception #130
Conversation
Thanks! |
Could you make a release? |
Embarassingly, my Haskell setup is currently broken somehow (looks like this https://discourse.nixos.org/t/stack-build-fails-with-startprocess-invalid-argument-bad-file-descriptor/1668 ). I'll do a release as soon as I have a second to fix it. |
This seems to work (if you excuse my unpinned
If that doesn't solve it, I've had some success clearing the |
Done, removing |
| CppNonStdException CppExceptionPtr (Maybe ByteString) | ||
|
||
instance Show CppException where | ||
showsPrec p (CppStdException _ msg typ) = showParen (p >= 11) (showString "CppStdException e " . showsPrec 11 msg . showsPrec 11 typ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberth Was it intended that there's this e
in the message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I get it. The e
is suppoed to represent the _
(first field of constructor CppStdException
) which is not show
n because it's a pointer.
@@ -275,16 +297,16 @@ main = Hspec.hspec $ do | |||
tag :: C.CppException -> String | |||
tag (C.CppStdException {}) = "CppStdException" | |||
tag (C.CppHaskellException {}) = "CppHaskellException" | |||
tag (C.CppOtherException {}) = "CppStdException" | |||
tag (Legacy.CppOtherException {}) = "CppStdException" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be "CppOtherException"
before and after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also have been pointed out by -Wall
for the test suite.
Fixed in my propsed PR #154
Thanks @nh2 for fixing the |
Currently, exceptions are caught by the
Cpp.Exceptions
module, where the original exception is thrown away. While this can be circumvented at a small scale by catching certain exceptions in C++ code, this is not feasible at a large scale, as it goes against what exceptions are intended for: make error handling implicit and centralize it around a few high-level call sites.This PR fixes this problem by using
std::exception_ptr
to represent C++ exceptions as first-class Haskell values.As a result, the caller can write their own exception handling function, to extract valuable information from specific exceptions. For example:
Review and implementation notes
The second commit renames a file, making the combined diff unhelpful. The first commit contains all the actual changes.
I've decided to move the code from
Exceptions
toException
so thatCppException
representation#include <exception>
andimport Control.Exception
; both singularI've decided to keep the string representations of the message and exception type, in order to avoid calling C++ code in the
Show
instance. This can be a little inefficient, but much better than storing these in[Char]
like we did.I've decided to make the exception module(s) depend on C++11. I believe requiring a decade old standard should not pose a problem. The alternative is to revive the old code as an independent module, but until we know that such a maintenance overhead is useful to anyone, this would be a wasted effort and a liability.