Exception Handling #1044

Merged
merged 24 commits into from Aug 14, 2016

Projects

None yet

4 participants

@rajithv
Contributor
rajithv commented Jul 23, 2016

An example with basic_div method.

@rajithv rajithv Init exception handling
13cbcb4
@rajithv rajithv changed the title from Exception Handling to [WIP] Exception Handling Jul 23, 2016
@isuruf isuruf and 1 other commented on an outdated diff Jul 23, 2016
symengine/cwrapper.cpp
{
- s->m = SymEngine::div(a->m, b->m);
+ try
+ {
+ s->m = SymEngine::div(a->m, b->m);
+ return 0;
+ }
+ catch ( ... )
@isuruf
isuruf Jul 23, 2016 Member

In SymEngine C++ layer, only std::runtime_errors are thrown, instead we can specialize std::runtime_errors and have SymEngine exceptions like ZeroDivisionError and in c wrapper we can return an error code for ZeroDivisionError.

In ruby you can check the error code and if it matches ZeroDivisionError you can throw a Ruby ZeroDivisionError, otherwise a generic Ruby exception

@rajithv
rajithv Jul 23, 2016 Contributor

got it.. generally what are the other errors you'd like to have specialized in the SymEngine?

I'm thinking of ParseError for parsing and MatrixError for matrices.

So, the general structure of exception handling is okay.. right? The parts in the Ruby wrappers as well? If so I can go ahead and include similar exception handling code for all the wrapped functions. (Except for those which are expected to return some values, thinking of the best way to handle them now)

@isuruf
isuruf Jul 23, 2016 Member

NotImplementedError for not implemented things.
DomainError for wrong domains, like evaluating complexes in real domain.
I think you will need to add more when you go through the codebase.

@isuruf
Member
isuruf commented Jul 23, 2016

@certik, this uses error codes as return values. One problem with this is that the messages are discarded and no stacktrace is given. There are 3 options,

  1. Keep this as it is and store the exception message in a thread local variable if supported or else in a global variable.
  2. Same as 2, but we don't store the error message if thread_local is not supported and thread safe mode is turned on.
  3. Pass in a pointer to a struct which can be NULL.

@rajithv, @abinashmeher999, let me know your thoughts on this.

@certik
Contributor
certik commented Jul 24, 2016

This looks good, and there should also be a test.

@certik
Contributor
certik commented Jul 24, 2016 edited

Good point about the stacktrace.

I think we should create macros, so that the function body would look like:

CWRAPPER_BEGIN
s->m = SymEngine::div(a->m, b->m);
CWRAPPER_END

and then we can easily change the implementation of these macros in all functions at once.

Do we want the exceptions to propagate to user code in Python/Ruby? For example, in Debug mode, we can let these macros print the full stacktrace.

The way I see it is that we should probably also create a macro that will call the C functions like this:

CWRAPPER_CHECK(basic_div(...))

So that we have the freedom to change if we return an int, or a pointer (in which case the user code must deallocate it if it is non-null).

In Python/Ruby wrappers, we can have a similar macro or a function, that will transform the exception into a Python or a Ruby exception.

@isuruf
Member
isuruf commented Jul 24, 2016

We can have the stacktrace printed even after catching the exception. (By having custom exceptions that store the stacktrace when the exception is constructed)

@certik
Contributor
certik commented Jul 24, 2016 edited

@isuruf the problem with constructing the stacktrace when the exception is created is that it is very slow. It's true that exceptions should only be raised rarely, but they still need to be reasonably fast.

@isuruf
Member
isuruf commented Jul 24, 2016

Yes, this can be turned on in debug mode if WITH_BFD set to yes

rajithv added some commits Jul 24, 2016
@rajithv rajithv Macros for exception handling
efe0630
@rajithv rajithv Basic runtime exception handling for most of the wrappers
27516e0
@certik certik and 2 others commented on an outdated diff Jul 24, 2016
symengine/cwrapper.cpp
@@ -58,6 +58,25 @@ inline bool is_aligned(T *p, size_t n = alignof(T))
extern "C" {
+
+#define CWRAPPER_BEGIN() \
+ try \
+ {
+
+#define CWRAPPER_END(error, code) \
+ return 0; \
+ } \
+ catch ( error ) \
+ { \
+ return code; \
+ }
+
+#define CWRAPPER_END_EXTRA(error, code) \
@certik
certik Jul 24, 2016 Contributor

This doesn't seem to be used anywhere.

@rajithv
rajithv Jul 24, 2016 Contributor

@certik I'm going to make SymEngine specific exceptions, and add them to relevant wrappers. (Eg: DivisionByZero for div, different types of ParseError for parse etc.). I'm yet to add them. I will include this also in the next commit.

@isuruf suggested that doing this and returning different error codes, so that in the Ruby Wrapper we can show error messages which makes more sense to the user.

@isuruf
isuruf Jul 25, 2016 Member

DivisionByZero is not found in only div, it can be thrown in any of the other functions that call div

@rajithv
rajithv Jul 25, 2016 Contributor

@isuruf yes.. I overlooked that. So do you suggest that I add catch statements for all the exceptions in the CWRAPPER_END macro, and get rid of the CWRAPPER_END_EXTRA macro?

@certik
certik Jul 25, 2016 Contributor

In the first iteration, I would get rid of CWRAPPER_END_EXTRA and just catch everything. We can refine it later, if we figure out a good way.

@rajithv
rajithv Jul 25, 2016 Contributor

I removed it and made a static CWRAPPER_END macro with all the current errors (actually DivisionByZero only). Will add more errors soon. Can you please check the new commit, especially the symengine_exception.cpp file?

@certik certik commented on an outdated diff Jul 24, 2016
symengine/cwrapper.cpp
s->m = SymEngine::symbol(std::string(c));
+ CWRAPPER_END(...,-1);
@certik
certik Jul 24, 2016 Contributor

Since all the wrappers use CWRAPPER_END(...,-1), why not to make it the default, and just call CWRAPPER_END().

Regarding the empty (), I think you can remove those too from both the definition and the call.

@certik certik and 1 other commented on an outdated diff Jul 24, 2016
symengine/cwrapper.cpp
@@ -170,34 +189,46 @@ TypeID basic_get_type(const basic s)
return static_cast<TypeID>(s->m->get_type_code());
}
-void symbol_set(basic s, const char *c)
+int symbol_set(basic s, const char *c)
@certik
certik Jul 24, 2016 edited Contributor

Regarding the output type, how about calling it CWRAPPER_OUTPUT_TYPE (perhaps you can find a better name that is not so long).

This output variable (be it void, int or ExceptionType * or void *) is only touched by the CWRAPPER_END macros. So if we instead use a CWRAPPER_OUTPUT_TYPE macro, then we have the freedom to change from void (the current master) to int or even to a pointer and allocate a struct with the exception info. All this by only modifying the macros, without touching the rest of the C wrappers.

What do you think?

@rajithv
rajithv Jul 24, 2016 Contributor

That's a really good idea. I was thinking of it, because now it's hard to distinguish between the instances of Exception Handling, and instances where an int is returned. I'll change that in the next commit.

@rajithv rajithv referenced this pull request in symengine/symengine.rb Jul 24, 2016
Merged

Exception Handling #64

rajithv added some commits Jul 25, 2016
@rajithv rajithv SymEngine Exception classes
8a6cd35
@rajithv rajithv Removing CWRAPPER_END_EXTRA
8a37dc9
@certik
Contributor
certik commented Jul 25, 2016

I think that looks good so far. If you could implement the CWRAPPER_OUTPUT_TYPE, that would be great. We can have a different int based on the exception type and then raise the appropriate exception in the Python/Ruby wrappers. I think that might be better than creating some temporary object that the wrappers must deallocate.

@isuruf isuruf commented on an outdated diff Jul 25, 2016
symengine/CMakeLists.txt
@@ -84,6 +84,7 @@ set(HEADERS
real_mpfr.h complex_mpc.h type_codes.inc lambda_double.h series.h series_piranha.h
basic-methods.inc series_flint.h series_generic.h sets.h derivative.h polys/upolybase.h
polys/uintpoly_flint.h polys/uintpoly_piranha.h eval.h flint_wrapper.h polys/basic_conversions.h
+ symengine_exception.h
@isuruf
isuruf Jul 25, 2016 Member

you can also do exceptions.h without the symengine_ prefix

@isuruf
Member
isuruf commented Jul 25, 2016

Instead of ints it's better to use enums

@abinashmeher999 abinashmeher999 and 1 other commented on an outdated diff Jul 27, 2016
symengine/symengine_exception.h
@@ -0,0 +1,16 @@
+#ifndef SYMENGINE_EXCEPTION_H
+#define SYMENGINE_EXCEPTION_H
+
+namespace SymEngine
+{
+
+class DivisionByZero: public std::exception
+{
+ virtual const char* what() const throw()
+ {
+ return "Division By Zero";
+ }
@abinashmeher999
abinashmeher999 Jul 27, 2016 Contributor

I am confused. Shouldn't the function be defined only once? And there are two namespaces symengine and SymEngine.

@rajithv
rajithv Jul 27, 2016 Contributor

.cpp file isn't being used. I deleted it. Thanks for spotting.

@rajithv rajithv CWRAPPER_OUTPUT_TYPE implemented
fe0086c
@certik
Contributor
certik commented Jul 27, 2016 edited

Where is CWRAPPER_OUTPUT_TYPE defined as int? I didn't find it, but apparently the code compiled somehow. Update: I see, I found it here: https://github.com/symengine/symengine/pull/1044/files#diff-b38693ebd058b160ac6e22b3812a9b1aR29, never mind.

Also the tests fail, I think they test for the runtime_error exception, so they need to be changed to test for this new DivisionByZero exception.

Otherwise it starts to look very good.

@certik certik commented on an outdated diff Jul 27, 2016
symengine/cwrapper.h
#endif // HAVE_SYMENGINE_MPC
//! Returns signed long value of s.
signed long integer_get_si(const basic s);
//! Returns unsigned long value of s.
unsigned long integer_get_ui(const basic s);
//! Returns s as a mpz_t.
-void integer_get_mpz(mpz_t a, const basic s);
+int integer_get_mpz(mpz_t a, const basic s);
@certik
certik Jul 27, 2016 Contributor

You forgot to change this int to CWRAPPER_OUTPUT_TYPE.

@rajithv rajithv All Division by Zero errors to be thrown as DivisioByZero, fixed test…
…s to reflect the change
18eb68b
@isuruf isuruf and 2 others commented on an outdated diff Jul 28, 2016
symengine/series.h
@@ -187,7 +188,7 @@ class SeriesBase : public SeriesCoeffInterface
unsigned int prec)
{
if (s == 0)
- throw std::runtime_error("Series::series_invert: division by zero");
+ throw DivisionByZero();
@isuruf
isuruf Jul 28, 2016 Member

Can you change DivisiionByZero so that a message like "Series::series_invert: division by zero" can be stored with the current message as the default?

@rajithv
rajithv Jul 28, 2016 Contributor

Can be done, but the problem is how to send that to the Ruby Wrapper. Shall I create a child class of the DivisionByZero?
Eg: DivisionByZeroSeriesInvert or something similar.

@certik
certik Jul 28, 2016 Contributor

The question is how much information we want to send to the wrappers. Generally, when division by zero occurs, either it's trivial (like if the user writes x/0), or it's inside some algorithm in SymEngine, in which case that algorithm needs to be fixed. We can print the nice full stacktrace (with exception string) if an exception is caught in the C wrappers in Debug mode. So the user just needs to turn on Debug mode to see what is going on. In Release mode, perhaps just a simple integer is all we need to return, i.e. as you already do in the PR?

rajithv added some commits Jul 29, 2016
@rajithv rajithv Added more common exceptions to symengine_exception.h
cd9414c
@rajithv rajithv Moved exceptions into one common class, added enums for error codes, …
…and a message for customized errors
6de56f0
@abinashmeher999 abinashmeher999 commented on an outdated diff Jul 29, 2016
symengine/symengine_exception.h
@@ -0,0 +1,34 @@
+#ifndef SYMENGINE_EXCEPTION_H
+#define SYMENGINE_EXCEPTION_H
+
+enum symengine_exceptions_t {
+ NO_EXCEPTION,
@abinashmeher999
abinashmeher999 Jul 29, 2016 Contributor

Can you explicitly set this to 0? So that the usual way of writing the function inside the if condition works everytime.

@abinashmeher999 abinashmeher999 and 1 other commented on an outdated diff Jul 29, 2016
symengine/symengine_exception.h
@@ -0,0 +1,34 @@
+#ifndef SYMENGINE_EXCEPTION_H
+#define SYMENGINE_EXCEPTION_H
+
+enum symengine_exceptions_t {
+ NO_EXCEPTION,
+ DIV_BY_ZERO,
+ NOT_IMPLEMENTED,
+ UNDEFINED
+};
+
@abinashmeher999
abinashmeher999 Jul 29, 2016 Contributor

The enum is defined here and this file is included in cwrapper.cpp, right? The C API user includes the cwrapper.h file. So when the C API returns an error code what will the error code be compared against?

@rajithv
rajithv Jul 29, 2016 Contributor

I will include it in cwrapper.h

@certik certik commented on the diff Jul 29, 2016
symengine/symengine_exception.h
+ NO_EXCEPTION,
+ DIV_BY_ZERO,
+ NOT_IMPLEMENTED,
+ UNDEFINED
+};
+
+namespace SymEngine
+{
+
+class SymEngineException : public std::exception
+{
+ std::string m_msg;
+ symengine_exceptions_t ec;
+
+public:
+ SymEngineException(const std::string &msg, symengine_exceptions_t error)
@certik
certik Jul 29, 2016 Contributor

Instead of passing it as an argument like this, why not to subclass SymEngineException with exception classes like DivisionByZero, NotImplemented, Undefined. And make error_code virtual, that way the CWrapper macros can stay unchanged (you just check for SymEngineException and call error_code, just like before). But in the C++ code, you can catch a particular exception. For example in the test, you can test the actual exception, not just the general SymEngineException, which can be anything.

I think just like in Python, one should try to catch the most particular exception.

@rajithv
rajithv Jul 30, 2016 Contributor

@certik I changed it. Can you please check and confirm whether this is what you had in mind? Then I can go ahead and implement the other exception classes.

@isuruf
isuruf Jul 30, 2016 Member

Looks good to me

@certik
certik Jul 31, 2016 Contributor

@rajithv yes, that's exactly what I had in mind. Thanks for doing it, I think it looks very good now.

@isuruf isuruf commented on an outdated diff Jul 30, 2016
symengine/symengine_exception.h
@@ -1,12 +1,14 @@
#ifndef SYMENGINE_EXCEPTION_H
#define SYMENGINE_EXCEPTION_H
-enum symengine_exceptions_t {
- NO_EXCEPTION,
+typedef enum {
+ NO_EXCEPTION = 0,
DIV_BY_ZERO,
NOT_IMPLEMENTED,
UNDEFINED
@isuruf
isuruf Jul 30, 2016 Member

Since this is C where there are no namespaces, it is better to use SYMENGINE_ prefix.

rajithv added some commits Jul 30, 2016
@rajithv rajithv Specialized exceptions, enum included in cwrappers
81ff094
@rajithv rajithv ParseErrors for exception handling
53d89c4
@certik
Contributor
certik commented Jul 31, 2016

Regarding the names of exceptions, should we follow what Python does (https://docs.python.org/3/library/exceptions.html#concrete-exceptions) and append Error in their name? E.g. NotImplementError and so on.

It looks like Ruby (http://ruby-doc.org/core-2.1.1/Exception.html) is also following the same naming scheme.

rajithv added some commits Jul 31, 2016
@rajithv rajithv Merge branch 'master' of https://github.com/symengine/symengine into …
…exception
f9e4be7
@rajithv rajithv Renaming exceptions
f2b1452
@rajithv rajithv referenced this pull request in symengine/symengine.rb Jul 31, 2016
Open

Ruby wrapper for parser #60

@abinashmeher999
Contributor

The Error Codes in C are usually the abbreviated form of the actual error and begin with an E like EINVAL for Invalid arguments, EUNDEF for undefined. The current names of Error codes in C++ is OK but try to make it more C like when you make the equivalent enum for the C wrappers.

@rajithv
Contributor
rajithv commented Aug 3, 2016

@abinashmeher999 there are many examples using the model you mentioned (eg: http://www.virtsync.com/c-error-codes-include-errno). But personally I think it is harder to understand. Should I prefix all the errors with ESYMENGINE_ or something like that? which would stand for SymEngine Errors. So, Division by Zero error would be ESYMENGINE_DIV_BY_ZERO what do you think? Also, I think it's better to keep the same name for enum in both C and C++.. isn't it?

@abinashmeher999
Contributor

@rajithv It will be a little less readable. Agreed. Keeping the same name for enum will be good but C ones will have to have a prefix to compensate lack of namespacing. ESYMENGINE_ seems a very long prefix to me. IMHO a shorter one like ESYM_ would be better.

@certik
Contributor
certik commented Aug 3, 2016

I like the current names:

+    SYMENGINE_NO_EXCEPTION = 0,
+    SYMENGINE_DIV_BY_ZERO = 1,
+    SYMENGINE_NOT_IMPLEMENTED = 2,
+    SYMENGINE_UNDEFINED = 3,
+    SYMENGINE_PARSE_ERROR = 4, 

Why not to keep it as it is?

rajithv added some commits Aug 4, 2016
@rajithv rajithv NotImplementedError tests
d6d04ba
@rajithv rajithv SymEngine Exceptions for Errors with specialized messages
8a2283d
@abinashmeher999
Contributor
abinashmeher999 commented Aug 4, 2016 edited

We can keep it as it is too. It's just that the identifiers starting with E are assumed as error codes and helps while scanning a peace of code. The current scheme is good.

@isuruf
Member
isuruf commented Aug 4, 2016

Can you fix the error?

/home/travis/build/symengine/symengine/symengine/series_piranha.cpp:68:11: error: ‘NotImplementedError’ is not a member of ‘std’
     throw std::NotImplementedError("Not Implemented");

Also you'll have to resolve conflicts.

@rajithv
Contributor
rajithv commented Aug 4, 2016

@isuruf I fixed the error. It's coming up with the next commit. Just saw the new conflicts. Will resolve that now.

rajithv added some commits Aug 4, 2016
@rajithv rajithv All runtime_errors converted to SymEngineExceptions 25ef8fc
@rajithv rajithv Merge branch 'master' of https://github.com/symengine/symengine into …
…exception
8bfe4ec
@certik
Contributor
certik commented Aug 4, 2016

@rajithv great job. +1 to merge as it is, if tests pass. We can improve upon it, if needed, after it's merged.

@certik
Contributor
certik commented Aug 4, 2016

Now it fails with an error:

/home/travis/build/symengine/symengine/symengine/tests/basic/test_functions.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____2659()’:

/home/travis/build/symengine/symengine/symengine/tests/basic/test_functions.cpp:2746:5: error: expected type-specifier before ‘SymEngineException’

Anyway, let's just fix these failures and merge.

@rajithv rajithv Fixing errors
e9ac6c2
@rajithv
Contributor
rajithv commented Aug 5, 2016

It's weird that I didn't get it on my local build. Anyway pushed it now. hope this time it'll pass.

@rajithv
Contributor
rajithv commented Aug 5, 2016

It's again failing in some specific configurations. Looks like I haven't put using SymEngine::SymEngineException in all the source files which need it.

@certik
Contributor
certik commented Aug 7, 2016

@rajithv, do you need some help with finishing it up? Let's get this merged, so that we can start using it.

rajithv added some commits Aug 12, 2016
@rajithv rajithv Fixing errors in tests c0a0b73
@rajithv rajithv Merge branch 'master' of https://github.com/symengine/symengine into …
…exception
7680322
@rajithv
Contributor
rajithv commented Aug 12, 2016

@certik looks like the checks are passing. Let's merge?

@isuruf isuruf commented on the diff Aug 12, 2016
symengine/tests/polynomial/test_uratpoly_flint.cpp
@@ -3,7 +3,9 @@
#include <symengine/polys/uintpoly_flint.h>
#include <symengine/pow.h>
+#include <symengine/symengine_exception.h>
@isuruf
isuruf Aug 12, 2016 Member

You don't need this include everywhere because it is included in basic.h and basic.h is included in almost all files.

@isuruf isuruf commented on the diff Aug 12, 2016
symengine/tests/polynomial/test_uratpoly_flint.cpp
+using SymEngine::SymEngineException;
@isuruf
isuruf Aug 12, 2016 Member

SymEngineException can be renamed to just Exception since we are using it inside SymEngine namespace.

@isuruf isuruf changed the title from [WIP] Exception Handling to Exception Handling Aug 12, 2016
@isuruf
Member
isuruf commented Aug 12, 2016

Left 2 minor comments (we can even do that later). I'm +1 to merge.

@certik certik merged commit 8629722 into symengine:master Aug 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@certik
Contributor
certik commented Aug 14, 2016

Thanks, I merged it. We can improve upon it in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment