PyException cleanup #2597
PyException cleanup #2597
Conversation
|
|
||
| void | ||
| IcePy::PyException::raiseLocalException() |
There was a problem hiding this comment.
merged raiseLocalException and raise
|
|
||
| if (PyObject_IsInstance(ex.get(), userExceptionType)) |
There was a problem hiding this comment.
We no longer handel user exceptions in raise. TypedUpcall::exception always marshall user exceptions, and Invocation::unmarshalException unmarshall exceptions and validates exception specificiation. A user exception in PyException::raise doesn't have a special meaning.
python/modules/IcePy/Util.cpp
Outdated
| } | ||
| else | ||
|
|
||
| string createUnknownExceptionMessage(PyObject* ex, PyObject* type, PyObject* tb, bool includeStackTrace) |
There was a problem hiding this comment.
I don't understand why we have an includeStackTrace parameter here (and by extension, the type and tb parameters).
The message for an UnknownException never includes the stack trace.
There was a problem hiding this comment.
The intention was to include it when it is raised by the application, and not include it when it is raised by the remote peer.
python/modules/IcePy/Util.cpp
Outdated
| throw Ice::UnknownLocalException{ | ||
| __FILE__, | ||
| __LINE__, | ||
| createUnknownExceptionMessage(ex.get(), _type.get(), _tb.get(), includeStackTrace)}; |
There was a problem hiding this comment.
We should never include the stack trace here.
python/modules/IcePy/Util.cpp
Outdated
| throw Ice::UnknownException{ | ||
| __FILE__, | ||
| __LINE__, | ||
| createUnknownExceptionMessage(ex.get(), _type.get(), _tb.get(), includeStackTrace)}; |
python/modules/IcePy/Util.h
Outdated
| @@ -89,7 +89,7 @@ namespace IcePy | |||
| // | |||
| // Convert the Python exception to its C++ equivalent. | |||
| // | |||
| void raise(); | |||
| void raise(bool includeStackTrace); | |||
There was a problem hiding this comment.
From reading the code, that's not really what raise() does. It converts the Python exception into one of the 6 special Ice local exceptions. And as a result, we don't need an includeStackTrace parameter for this function.
|
Rewoked to never include stack trace with unknown exceptions. |
python/modules/IcePy/Util.cpp
Outdated
| } | ||
|
|
||
| void | ||
| IcePy::PyException::raise() | ||
| namespace IcePy |
There was a problem hiding this comment.
These should use the same style as the rest:
string
IcePy::getTypeName(...
There was a problem hiding this comment.
I move them to the .cpp file because they are local to this file. Updated the code to use tne anonymous namespace instead of IcePy to make it clear.
python/modules/IcePy/Util.cpp
Outdated
| } | ||
|
|
||
| void | ||
| IcePy::PyException::raise() | ||
| namespace IcePy |
There was a problem hiding this comment.
Did you intend to use the anonymous namespace, and not reopen IcePy?
As far as I can tell, these functions are local to this file and not declared in the header.
No description provided.