wmayer wrote:The actual problem is how DocumentObject::execute(void) is implemented. It calls the extensionExecute of its extensions but doesn't stop and forward the DocumentObjectExecReturn on errors. Furthermore it causes a memory leak because the result object gets never destroyed.
So, after the merge (thanks!), there is still a memory leak in the case of an OCC exception (see StandardFailure catch block):
https://github.com/FreeCAD/FreeCAD/pull ... 52df1a91f0
Is not it?
What is the meaning of DocumentObjectExecReturn? I see that it stores an object and a string from the declaration. I would appear that it wants to be a substitution mechanism for an exception. You gain knowing the object to which the string text is related. You lose knowing the type of exception.
Part of the question is related also to: Do the extensions need to handle DocumentObjectExecReturns? It is difficult for me to understand this without know the answer to the previous question, but it would appear that DocumentObjectExecReturns is the responsibility of the DocumentObject, a responsibility not (necessarily) to be delegated to the extensions.
Shouldn't DocumentObject just catch whatever exception coming from the extensions and fill the structure as needed?
wmayer wrote:A direct dependency of the core system on OCC is not desired. If we want some special OCC handling there is no problem to introduce a new exception class.
On feature level OCC exceptions are converted into a DocumentObjectExecReturn structure.
I have seen that you have extended the number of exception classes in a separate commit. I welcome this very much.
RELATED TO EXCEPTIONS
Lately I have found some issues in the exception system that makes me less productive as a coder and generally come with a wrong or reduced information to the end user. I have been working into trying to improve the exceptions so that:
1. The exception includes a message intended to be shown to the user and potentially translated. This message does not contain any other information used upstream in the exception throwing and catching or for interaction with Python. There is no need to strip this string from prepended prefixes/suffixes. This message is usable directly for the Python Console AND the UI.
2. The exception includes a higher amount of information about in which function, file and line it was thrown. This has to be harvested automatically. This information is shown in the Report view and is mainly intended to allow developers to track complex workflows and improve the filling of bug reports.
This branch is to show what I have done so far (work in progress), so that you can give me your opinion:
https://github.com/abdullahtahiriyo/Fre ... provements
I do not mean it to be
the solution. It may be the solution or a way to discuss it and gather further user/developer feedback in order to arrive to a better solution.
What I have done in words
1. I extended the exception class to hold separate data member fields for a translatable message, a prefix (probably deemed to be removed, as it seemed necessary for python interfacing, but now I think it won't be necessary), file and line fields for the exception.
2. I created macros to throw exceptions, so that function, file and line is automatically retrieved by the preprocesor/compiler and inserted into the exception. THROW to throw an exception without message, THROWM to throw an exception with a message but without a prefix, THROWMP to throw an exception with a message and a preffix.
The automatic harvesting of information is intended to avoid spoiling a translatable text message usable in the UI with function information and effectively automating it.
Classes that currently have a "what" with a mixture of translatable and non-translatable text (see FileDialogError exception, that has the string of the file name), still produce the same "what" for backwards compatibility, but include a separate data member to hold the translatable text portion.
How it behaves:
Practical example:
I modified, the sketcher modifyBsplineKnotMultiplicity to use the macros just for testing (do not try to give it meaning, it does not have one), like this:
Code: Select all
if (GeoId < 0 || GeoId > getHighestCurveIndex())
THROWM(Exception, "BSpline GeoId is out of bounds.")
//throw Base::ValueError("BSpline GeoId is out of bounds.");
if (multiplicityincr == 0) // no change in multiplicity
THROWMP(Exception, "You are requesting no change in knot multiplicity.", "trial")
//throw Base::ValueError("You are requesting no change in knot multiplicity.");
const Part::Geometry *geo = getGeometry(GeoId);
if(geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId())
THROW(Exception)
//throw Base::TypeError("The GeoId provided is not a B-spline curve");
This is how it behaves:
Python console:
App.ActiveDocument.Sketch004.modifyBSplineKnotMultiplicity(6,3,0)
Traceback (most recent call last):
File "<input>", line 1, in <module>
Base.FreeCADError: FreeCAD exception thrown (You are requesting no change in knot multiplicity.)
App.ActiveDocument.Sketch004.modifyBSplineKnotMultiplicity(16,3,0)
Traceback (most recent call last):
File "<input>", line 1, in <module>
Base.FreeCADError: FreeCAD exception thrown (BSpline GeoId is out of bounds.)
Report view:
Exception (Thu Apr 27 19:15:24 2017): BSpline GeoId is out of bounds. in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4102
Exception (Thu Apr 27 18:58:34 2017): trial You are requesting no change in knot multiplicity. in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4106
Exception (Thu Apr 27 19:25:07 2017): in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4113
Where I am now?
Well I am taking a look into the Python part.
After quite a lot of digging and debuging, I have understood (or I think I have) this process of converting the c++ exception into Python exception and the throwing the c++ exception after python execution.
An example of this process:
1. We run a command, that gets executed via " presult = PyRun_String(sCmd, Py_file_input, dict, dict); /* eval direct */" in "std::string InterpreterSingleton::runString(const char *sCmd)"
2. *Py uses PyErr_SetString to associate a string to a python exception (could also use PyErr_SetObject, PyErr_Format). Example
Code: Select all
catch(const Base::Exception& e) // catch the FreeCAD exceptions
{
std::string str;
str += "FreeCAD exception thrown (";
str += e.what();
str += ")";
e.ReportException();
PyErr_SetString(Base::BaseExceptionFreeCADError,str.c_str());
return NULL;
}
3. The result in 1 is evaluated and if there is none, the python exception is evaluated:
Code: Select all
if (!presult) {
if (PyErr_ExceptionMatches(PyExc_SystemExit))
throw SystemExitException();
else
throw PyException();
}
4. This PyException constructor, gets the information for Python via:
Code: Select all
PyException::PyException(void)
{
PP_Fetch_Error_Text(); /* fetch (and clear) exception */
std::string prefix = PP_last_error_type; /* exception name text */
// prefix += ": ";
std::string error = PP_last_error_info; /* exception data text */
5. Notably the exception information is obtained via PP_Fetch_Error_Text()
Code: Select all
void PP_Fetch_Error_Text()
{
// called without exception happened!
//assert(PyErr_Occurred());
char *tempstr;
PyObject *errobj, *errdata, *errtraceback, *pystring;
/* get latest python exception information */
/* this also clears the current exception */
PyErr_Fetch(&errobj, &errdata, &errtraceback); /* all 3 incref'd */
/* convert type and data to strings */
/* calls str() on both to stringify */
pystring = NULL;
if (errobj != NULL &&
(pystring = PyObject_Str(errobj)) != NULL && /* str(errobj) */
(PyString_Check(pystring)) ) /* str() increfs */
{
strncpy(PP_last_error_type, PyString_AsString(pystring), MAX); /*Py->C*/
PP_last_error_type[MAX-1] = '\0';
}
else
strcpy(PP_last_error_type, "<unknown exception type>");
Py_XDECREF(pystring);
pystring = NULL;
if (errdata != NULL &&
(pystring = PyObject_Str(errdata)) != NULL && /* str(): increfs */
(PyString_Check(pystring)) )
{
strncpy(PP_last_error_info, PyString_AsString(pystring), MAX); /*Py->C*/
PP_last_error_info[MAX-1] = '\0';
}
else
strcpy(PP_last_error_info, "<unknown exception data>");
Py_XDECREF(pystring);
/* convert traceback to string */
/* print text to a StringIO.StringIO() internal file object, then */
/* fetch by calling object's .getvalue() method (see lib manual); */
pystring = NULL;
if (errtraceback != NULL &&
(PP_Run_Function("StringIO", "StringIO", "O", &pystring, "()") == 0) &&
(PyTraceBack_Print(errtraceback, pystring) == 0) &&
(PP_Run_Method(pystring, "getvalue", "s", &tempstr, "()") == 0) )
{
strncpy(PP_last_error_trace, tempstr, MAX);
PP_last_error_trace[MAX-1] = '\0';
free(tempstr); /* it's a strdup */
}
else
strcpy(PP_last_error_trace, "<unknown exception traceback>");
Py_XDECREF(pystring);
Py_XDECREF(errobj);
Py_XDECREF(errdata); /* this function owns all 3 objects */
Py_XDECREF(PP_last_traceback); /* they've been NULL'd out in Python */
PP_last_traceback = errtraceback; /* save/export raw traceback object */
}
6. From this, the type of exception is obtained directly from PyErr_Fetch and
without taking into account the prepended string at all.
I have also noted that in different workbenches, different friends of SetString are used (at least Format in PD).
I see that there is the possibility of setting an error indicator as SetObject (another friend of SetString). Taking into consideration the changes I mention above to the c++ exception classes,
wouldn't it make sense to create an Object to hold all the exception information separately and avoid to try to code everything in a string? wouldn't it make sense to extend the types of python exceptions to have a one-to-one match with the c++ ones?
Sorry for the long post. If it looks like I am disoriented, then it is certainly because I am. Too many new things outside my Sketcher little parcel